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

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/11/7 19:08, Jan Beulich wrote:
>>>> 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.

Okay I will try to go there.

>
>> --- 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.

Do you mean this line?

copy_from_guest_offset(pcidevs, xdsr->pcidevs, 0, 
xdsr->num_pcidevs*sizeof(xen_guest_pcidev_info_t))

>
>> --- 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.

How about bool_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

Yes. Actually I ever considered this point but I just think we may need 
to keep a complete set of fields.

You're right and anywhere what we should do is focusing on on-demand.

> 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.

Okay I can use 'force' directly.

In Xen side we have 'bool_t', but we have 'bool' in tools side. So how 
to define this in xen/include/public/domctl.h?

>
>> +struct xen_domctl_set_rdm {
>> +    uint32_t  pci_rdmforce;
>
> Same comment as on the field added to "struct hvm_domain".

Ditto.

>
>> +    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.

Looks I should do this,

XEN_GUEST_HANDLE_64(xen_guest_pcidev_info_t) pcidevs;

>
>> @@ -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?

As I understand we should shrink this pad when we introduce new filed, 
shouldn't we?

Thanks
Tiejun