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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.11.14 at 08:49, <tiejun.chen@xxxxxxxxx> wrote:
>>>>>> --- 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.
>>
> 
> Are you saying this change?
> 
> @@ -898,14 +899,36 @@ int 
> intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
>   {
>       struct acpi_rmrr_unit *rmrr;
>       int rc = 0;
> +    int i, j;
> +    u16 bdf, pt_bdf;
> +    struct domain *d = ctxt->domain;
> 
> -    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> +    for_each_rmrr_device ( rmrr, bdf, i )
>       {
> -        rc = func(PFN_DOWN(rmrr->base_address),
> -                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
> -                  ctxt);
> -        if ( rc )
> -            break;
> +        if ( d->arch.hvm_domain.pci_force )
> +        {
> +            rc = func(PFN_DOWN(rmrr->base_address),
> +                      PFN_UP(rmrr->end_address) -
> +                      PFN_DOWN(rmrr->base_address),
> +                      ctxt);
> +            if ( rc )
> +                break;
> +        }
> +        else
> +        {
> +            for ( j = 0; j < d->arch.hvm_domain.num_pcidevs; j++ )
> +            {
> 
> But,
> 
> dmar.c: In function 'intel_iommu_get_reserved_device_memory'"
> dmar.c:904:28: error: dereferencing 'void *' pointer [-Werror]
>       struct domain *d = ctxt->domain;
>                              ^
> dmar.c:904:28: error: request for member 'domain' in something not a 
> structure or union
> cc1: all warnings being treated as errors
> make[6]: *** [dmar.o] Error 1
> make[6]: *** Waiting for unfinished jobs.
> 
> Unless we move all check inside each callback functions.

That's what you should do imo, albeit I realize that the comparing
of the specific SBDFs will need additional consideration.

Jan


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