Re: [PATCH v5 1/3] x86/iommu: remove regions not to be mapped

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/2024 12:37 pm, Roger Pau Monné wrote:
> On Thu, Jan 25, 2024 at 12:13:01PM +0100, Jan Beulich wrote:
>> On 25.01.2024 09:47, Roger Pau Monné wrote:
>>> On Thu, Jan 25, 2024 at 09:34:40AM +0100, Jan Beulich wrote:
>>>> On 24.01.2024 18:29, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/hvm/io.c
>>>>> +++ b/xen/arch/x86/hvm/io.c
>>>>> @@ -369,6 +369,22 @@ bool vpci_is_mmcfg_address(const struct domain *d, 
>>>>> paddr_t addr)
>>>>>      return vpci_mmcfg_find(d, addr);
>>>>>  }
>>>>>  
>>>>> +int __hwdom_init vpci_subtract_mmcfg(const struct domain *d, struct 
>>>>> rangeset *r)
>>>>> +{
>>>>> +    const struct hvm_mmcfg *mmcfg;
>>>>> +
>>>>> +    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
>>>>> +    {
>>>>> +        int rc = rangeset_remove_range(r, PFN_DOWN(mmcfg->addr),
>>>>> +                                       PFN_DOWN(mmcfg->addr + 
>>>>> mmcfg->size - 1));
>>>> Along the lines of this, ...
>>>>
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -2138,6 +2138,54 @@ int __hwdom_init xen_in_range(unsigned long mfn)
>>>>>      return 0;
>>>>>  }
>>>>>  
>>>>> +int __hwdom_init remove_xen_ranges(struct rangeset *r)
>>>>> +{
>>>>> +    paddr_t start, end;
>>>>> +    int rc;
>>>>> +
>>>>> +    /* S3 resume code (and other real mode trampoline code) */
>>>>> +    rc = rangeset_remove_range(r, 
>>>>> PFN_DOWN(bootsym_phys(trampoline_start)),
>>>>> +                               PFN_DOWN(bootsym_phys(trampoline_end)) - 
>>>>> 1);
>>>> ... did you perhaps mean
>>>>
>>>>                                PFN_DOWN(bootsym_phys(trampoline_end) - 1));
>>>>
>>>> here (and then similarly below, except there the difference is benign I
>>>> think, for the labels being page-aligned)?
>>> They are all page aligned, so I didn't care much,  but now that you
>>> point it might be safer to do the subtraction from the address instead
>>> of the frame number, just in case.
>> Hmm, no, for me neither trampoline_end nor trampoline_start are page
>> aligned. While bootsym_phys(trampoline_start) is, I don't think
>> bootsym_phys(trampoline_end) normally would be (it might only be by
>> coincidence).
> Oh, so it had been a coincidence of the build I was using I guess then.

trampoline_start has to be page aligned because of constraints from SIPI
and S3 (cant remember which one is the 4k constraint, but it's in the
comments).

On APs (and indeed, in Xen's pagetables), the trampoline is only a
single 4k page.

However, trampoline_end is quite a lot longer because there's various
things that get done on the BSP only, including recovering the E820 map,
EDID/etc in 16bit mode.

That said, we don't edit the trampoline very often, so if it happened to
work for you first time around, it probably hasn't changed since.

~Andrew