Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.11.14 at 11:27, <tiejun.chen@intel.com> wrote:
> Are you saying Xen restrict some BDFs specific to emulate some devices? 
> But I don't see these associated codes.

I didn't say so. All I said that some of the SBDFs are being used by
them.

>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -158,14 +158,14 @@ struct iommu_ops {
>>>        void (*crash_shutdown)(void);
>>>        void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
>>>        void (*iotlb_flush_all)(struct domain *d);
>>> -    int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
>>> +    int (*get_reserved_device_memory)(iommu_grdm_t *, struct domain *, void *);
>>>        void (*dump_p2m_table)(struct domain *d);
>>>    };
>>>
>>>    void iommu_suspend(void);
>>>    void iommu_resume(void);
>>>    void iommu_crash_shutdown(void);
>>> -int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
>>> +int iommu_get_reserved_device_memory(iommu_grdm_t *, struct domain *, void *);
>>
>> I don't see why these generic interfaces would need to change;
>> the only thing that would seem to need changing is the callback
>> function (and of course the context passed to it).
> 
> I'm not 100% sure if we can call current->domain in all scenarios. If 
> you can help me confirm this I'd really like to remove this change :) 
> Now I assume this should be true as follows:

Which is wrong, and not what I said. Instead you should pass the
domain as part of the context that gets made available to the
callback function.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1540,6 +1540,34 @@ int iommu_do_pci_domctl(
>           }
>           break;
> 
> +    case XEN_DOMCTL_set_rdm:
> +    {
> +        struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
> +        struct xen_guest_pcidev_info *pcidevs;
> +        int i;
> +
> +        pcidevs = xmalloc_array(xen_guest_pcidev_info_t,
> +                                domctl->u.set_rdm.num_pcidevs);
> +        if ( pcidevs == NULL )
> +        {
> +            return -ENOMEM;
> +        }
> +
> +        for ( i = 0; i < xdsr->num_pcidevs; ++i )
> +        {
> +            if ( __copy_from_guest_offset(pcidevs, xdsr->pcidevs, i, 1) )
> +            {
> +                xfree(pcidevs);
> +                return -EFAULT;
> +            }
> +        }

I don't see the need for a loop here. And you definitely can't use the
double-underscore-prefixed variant the way you do.

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -90,6 +90,10 @@ struct hvm_domain {
>       /* Cached CF8 for guest PCI config cycles */
>       uint32_t                pci_cf8;
> 
> +    uint32_t                pci_rdmforce;

I still don't see why this is a uint32_t.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -484,6 +484,24 @@ struct xen_domctl_assign_device {
>   typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
>   DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t);
> 
> +struct xen_guest_pcidev_info {
> +    uint8_t func;
> +    uint8_t dev;
> +    uint8_t bus;
> +    int domain;
> +    int rdmforce;
> +};

Please see struct physdev_pci_device_add for how to properly and
space efficiently express what you need. And of course I'd expect
the code to actually use all fields you specify here - neither domain
(which really ought to be named segment if it is what I think it is
meant to be) nor rdmforce are being used anywhere. Plus - again -
just "force" would be sufficient as a name, provided the field is
needed at all.

> +struct xen_domctl_set_rdm {
> +    uint32_t  pci_rdmforce;

Same comment as on the field added to "struct hvm_domain".

> +    uint32_t  num_pcidevs;
> +    XEN_GUEST_HANDLE(xen_guest_pcidev_info_t) pcidevs;

Did you _at all_ look at any of the other domctls when adding this?
There's not a single use of XEN_GUEST_HANDLE() in the whole file.

> @@ -1118,7 +1137,8 @@ struct xen_domctl {
>           struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
>           struct xen_domctl_vnuma             vnuma;
>           struct xen_domctl_psr_cmt_op        psr_cmt_op;
> -        uint8_t                             pad[128];
> +        struct xen_domctl_set_rdm           set_rdm;
> +        uint8_t                             pad[112];

Why are you altering the padding size here?

Jan