Re: [Xen-devel] [PATCH RFC] xen/pvh: use a custom IO bitmap for PVH hardware domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.04.15 at 14:57, <roger.pau@xxxxxxxxxx> wrote:
> Since a PVH hardware domain has access to the physical hardware create a
> custom more permissive IO bitmap. The permissions set on the bitmap are
> populated based on the contents of the ioports rangeset.
> 
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.

So why is what ns16550_endboot() does not sufficient?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>      hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>  
> +/* I/O permission bitmap for HVM hardware domain */
> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> +    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];

This should be allocated memory, as it's needed only in the PVH
Dom0 case.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
>  {
>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
>      struct vmcb_struct *vmcb = arch_svm->vmcb;
> +    struct domain *d = v->domain;

I don't see the value of this variable.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,10 @@ static int construct_vmcs(struct vcpu *v)
>      }
>  
>      /* I/O access bitmap. */
> -    __vmwrite(IO_BITMAP_A, virt_to_maddr((char *)hvm_io_bitmap + 0));
> -    __vmwrite(IO_BITMAP_B, virt_to_maddr((char *)hvm_io_bitmap + PAGE_SIZE));
> +    __vmwrite(IO_BITMAP_A,
> +              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + 0));
> +    __vmwrite(IO_BITMAP_B,
> +              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + 
> PAGE_SIZE));

Please drop the bogus cast and pointless addition of zero from the
first of these. I'd prefer it to be done without cast on the second one
too.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>              int bytes = (exit_qualification & 0x07) + 1;
>              int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> +
>              if ( handle_pio(port, bytes, dir) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }

This is a valid style correction, but being the only change to this file
it doesn't belong here.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
>  extern bool_t hvm_enabled;
>  extern bool_t cpu_has_lmsl;
>  extern s8 hvm_port80_allowed;
> +extern unsigned long hvm_hw_io_bitmap[];

This should be declared next to hvm_io_bitmap[] (I don't mind if
you move its declaration here).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel