Re: Xen PAT settings vs Linux PAT settings

[Marek Marczykowski-Górecki]

On Mon, Oct 14, 2024 at 09:05:58PM +0100, Andrew Cooper wrote:
> On 14/10/2024 7:26 pm, Marek Marczykowski-Górecki wrote:
> > Hi,
> >
> > It looks like we've identified the second buggy driver that somewhere
> > assumes PAT is configured as Linux normally do natively - nvidia binary
> > one this time[3]. The first one affected was i915, but it turned out to be
> > a bug in Linux mm. It was eventually fixed[1], but it was quite painful
> > debugging. This time a proper fix is not known yet. Since the previous
> > issue, Qubes OS carried a patch[2] that changes Xen to use same PAT as
> > Linux. We recently dropped this patch, since the Linux fix reached all
> > supported by us branches, but apparently it wasn't all...
> >
> > Anyway, would it be useful (and acceptable) for upstream Xen to have
> > a kconfig option (behind UNSUPPORTED or so) to switch this behavior?
> 
> Not UNSUPPORTED - it's bogus and I still want it purged.
> 
> But, behind EXPERT, with a suitable description (e.g. "This breaks
> various ABIs including migration, and is presented here for debugging PV
> driver issues in a single system.  If turning it on fixes a bug, please
> contact upstream Xen"), then I think we need to take it.

Makes sense.

> The fact that I've had to recommend it once already this week for
> debugging purposes, and it wasn't even this Nvidia bug, demonstrates how
> pervasive the problems are.
> 
> > Technically, it's a PV ABI violation, and it does break few things
> > (definitely PV domU with passthrough are affected - Xen considers them
> > L1TF vulnerable then; PV live migration is most likely broken too).
> 
> Do you have more information on this?  The PAT bits shouldn't form any
> part of L1TF considerations.

https://github.com/QubesOS/qubes-issues/issues/8593

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [RFC QEMU PATCH v7 1/1] xen/pci: get gsi for passthrough devices

[Chen, Jiqian]

On 2024/10/15 03:34, Stewart Hildebrand wrote:
> +Edgar
> 
> On 5/16/24 06:13, Jiqian Chen wrote:
>> In PVH dom0, it uses the linux local interrupt mechanism,
>> when it allocs irq for a gsi, it is dynamic, and follow
>> the principle of applying first, distributing first. And
>> the irq number is alloced from small to large, but the
>> applying gsi number is not, may gsi 38 comes before gsi
>> 28, that causes the irq number is not equal with the gsi
>> number. And when passthrough a device, qemu wants to use
>> gsi to map pirq, xen_pt_realize->xc_physdev_map_pirq, but
>> the gsi number is got from file
>> /sys/bus/pci/devices//irq in current code, so it
>> will fail when mapping.
>>
>> Get gsi by using new function supported by Xen tools.
>>
>> Signed-off-by: Huang Rui 
>> Signed-off-by: Jiqian Chen 
> 
> I think you can safely remove the RFC tag since the Xen bits have been
> upstreamed.
Thank you! I will send a new version later this week and modify the patch 
according to your comments.

> 
>> ---
>>  hw/xen/xen-host-pci-device.c | 19 +++
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 8c6e9a1716a2..2fe6a60434ba 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -10,6 +10,7 @@
>>  #include "qapi/error.h"
>>  #include "qemu/cutils.h"
>>  #include "xen-host-pci-device.h"
>> +#include "hw/xen/xen_native.h"
> 
> The inclusion order unfortunately seems to be delicate.
> "hw/xen/xen_native.h" should be before all the other xen
> includes, but after "qemu/osdep.h".
> 
>>  
>>  #define XEN_HOST_PCI_MAX_EXT_CAP \
>>  ((PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE) / (PCI_CAP_SIZEOF + 
>> 4))
>> @@ -329,12 +330,17 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice 
>> *d, uint32_t cap)
>>  return -1;
>>  }
>>  
>> +#define PCI_SBDF(seg, bus, dev, func) \
>> +uint32_t)(seg)) << 16) | \
>> +(PCI_BUILD_BDF(bus, PCI_DEVFN(dev, func
>> +
>>  void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>   uint8_t bus, uint8_t dev, uint8_t func,
>>   Error **errp)
>>  {
>>  ERRP_GUARD();
>>  unsigned int v;
>> +uint32_t sdbf;
> 
> Typo: s/sdbf/sbdf/
> 
>>  
>>  d->config_fd = -1;
>>  d->domain = domain;
>> @@ -364,11 +370,16 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, 
>> uint16_t domain,
>>  }
>>  d->device_id = v;
>>  
>> -xen_host_pci_get_dec_value(d, "irq", &v, errp);
>> -if (*errp) {
>> -goto error;
>> +sdbf = PCI_SBDF(domain, bus, dev, func);
>> +d->irq = xc_physdev_gsi_from_dev(xen_xc, sdbf);
> 
> This was renamed to xc_pcidev_get_gsi.
> 
> This also needs some sort of Xen interface version guard for backward
> compatibility since it's a new call introduced in Xen 4.20.
> 
>> +/* fail to get gsi, fallback to irq */
>> +if (d->irq == -1) {
>> +xen_host_pci_get_dec_value(d, "irq", &v, errp);
>> +if (*errp) {
>> +goto error;
>> +}
>> +d->irq = v;
>>  }
>> -d->irq = v;
>>  
>>  xen_host_pci_get_hex_value(d, "class", &v, errp);
>>  if (*errp) {
> 

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v3 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> Secondary cpus initialization is not yet supported. Thus, we print an
> appropriate message and put the secondary cpus in WFE state.
> And we introduce to BUILD_BUG_ON to prevent users using from building Xen
> on multiprocessor based MPU systems.
> 
> In Arm, there is no clean way to disable SMP. As of now, we wish to support
> MPU on UNP only. So, we have defined the default range of NR_CPUs to be 1 for
> MPU.
> 
> Signed-off-by: Ayan Kumar Halder 

Apart from Jan’s comment, the rest of the changes to the arch/arm part looks ok 
to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

> /*
>  * Maps the various sections of Xen (described in xen.lds.S) as different MPU
>  * regions.
> @@ -68,10 +92,11 @@
>  * Inputs:
>  *   lr : Address to return to.
>  *
> - * Clobbers x0 - x5
> + * Clobbers x0 - x6
>  *
>  */
> FUNC(enable_boot_cpu_mm)
> +mov   x6, lr
> 
> /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> mrs   x5, MPUIR_EL2
> @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm)
> beq 5f
> prepare_xen_region x0, x1, x2, x3, x4, x5
> 
> -5:
> +5:  mov   lr, x6

Shall these changes to enable_boot_cpu_mm be part of the previous commit?

The rest looks good to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Jan Beulich]

On 14.10.2024 18:32, Frediano Ziglio wrote:
> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
>  wrote:
>>
>> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>>> index 1199291d2b..23ad274c89 100644
>>> --- a/xen/arch/x86/boot/Makefile
>>> +++ b/xen/arch/x86/boot/Makefile
>>> @@ -1,4 +1,5 @@
>>>  obj-bin-y += head.o
>>> +obj-bin-y += built_in_32.o
>>
>> I don't quite like that this new object name is "built_in_32.o",
>> It's really closed to "built_in.*" which is used by Rules.mk to collect
>> all objects in a subdirectory. I don't really have a better suggestion at
>> the moment.
>>
> 
> It was cbundle.o before, but people preferred built_in_32.o.
> It's a collection of object files like built_in.o so it does not seem
> so bad to me.
> But seen other replies, some other people prefer "bundle".

Well, I for one don't really _prefer_ bundle. I merely see it as a possible
option to address Anthony's name ambiguity concern.

>>> + $(LD32) $(orphan-handling-y) -N -T $< -Map 
>>> $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o 
>>> $(obj)/built_in_32.tmp.o
>>
>> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst 
>> %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
>>
>> :-(, maybe that's lots of $(patsubst,), not sure which is better between
>> $(patsubst,) and using the stem $*.
>>
> 
> Not strong about stem or patsubst.
> The 2 filters seem good, they suggest lds for the script and objects
> for the input, which makes sense.
> 
>> Also, if something tries to use built_in_32.tmp.bin, we have a rule that
>> remove it's prerequisite.
>>
>> BTW, everything is kind of temporary in a build system, beside the few
>> files that we want to install on a machine, so having a target named
>> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
>> remove them before the end of its recipe is fine (or those *tmp* aren't
>> use outside of this recipe).
>>
> 
> Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
> So we still don't agree on one name, and now we want to find also
> another, tricky.
> More or less if it can help, one is a 32 bit object file that bundle
> together multiple 32 bits object files while the other is the
> conversion of the 32 bits bundle file to 64 bits.
> XXX_32.o and XXX_32as64.o ??

Whatever the eventual name (I don't care all that much), just one request:
Dashes instead of underscores please.

Jan

Re: [PATCH v2 1/2] xen/riscv: initialize bootinfo from dtb

[Jan Beulich]

On 10.10.2024 17:30, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>_end - _start, false) )
>  panic("Failed to add BOOTMOD_XEN\n");
>  
> +BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));

We generally aim at avoiding side effects in BUG_ON() (or ASSERT()). With

if (!boot_fdt_info(device_tree_flattened, dtb_addr))
BUG();

Acked-by: Jan Beulich 

I can make the adjustment while committing, if desired.

Jan

Re: [PATCH v9 1/2] x86/boot: Align mbi2.c stack to 16 bytes

[Jan Beulich]

On 10.10.2024 11:45, Frediano Ziglio wrote:
> Doing previous testing with an Alder Lake Intel machine the with

Nit: flip "the" and "with"?

> "x86/boot: Improve MBI2 structure check" commit test started to fail.
> Removing the commit makes the tests succeed however there was not apparent
> reason (looking at the code) for the failure.
> So I instrumented code to output the structure and tested code with
> this extracted data with and without the mentioned commit and results
> were the same.
> Compiled assembly code from lab was also fine beside not keeping
> the 16-byte alignment for the stack.
> Turning on stack alignment solve the problem on Alder Lake machine.
> 
> Fixes: eb21ce14d709 ('x86/boot: Rewrite EFI/MBI2 code partly in C')
> Signed-off-by: Frediano Ziglio 

Reviewed-by: Jan Beulich 

Re: [PATCH] iommu/amd-vi: do not error if device referenced in IVMD is not behind any IOMMU

[Jan Beulich]

On 09.10.2024 15:37, Roger Pau Monné wrote:
> On Wed, Oct 09, 2024 at 02:09:33PM +0200, Jan Beulich wrote:
>> On 09.10.2024 13:47, Roger Pau Monné wrote:
>>> On Wed, Oct 09, 2024 at 01:28:19PM +0200, Jan Beulich wrote:
 On 09.10.2024 13:13, Roger Pau Monné wrote:
> I also think returning an error when no device in the IVMD range is
> covered by an IOMMU is dubious.  Xen will already print warning
> messages about such firmware inconsistencies, but refusing to boot is
> too strict.

 I disagree. We shouldn't enable DMA remapping in such an event. Whereas
>>>
>>> I'm not sure I understand why you would go as far as refusing to
>>> enable DMA remapping.  How is a IVMD block having references to some
>>> devices not assigned to any IOMMU different to all devices referenced
>>> not assigned to any IOMMU?  We should deal with both in the same
>>> way.
>>
>> Precisely because of ...
>>
>>> If all devices in the IVMD block are not covered by an IOMMU, the
>>> IVMD block is useless.
>>
>> ... this. We simply can't judge whether such a useless block really was
>> meant to cover something. If it was, we're hosed. Or maybe we screwed up
>> and wrongly conclude it's useless.
> 
> The same could be stated about devices in a IVMD block that are not
> assigned to an IOMMU: it could also be Xen that screwed up and wrongly
> concluded they are not assigned to an IOMMU.
> 
>>>  But there's nothing for Xen to action, due to
>>> the devices not having an IOMMU assigned.  IOW: it would be the same
>>> as booting natively without parsing the IVRS in the first place.
>>
>> Not really, no. Not parsing IVRS means not turning on any IOMMU. We
>> then know we can't pass through any devices. We can't assess the
>> security of passing through devices (as far as it's under our control)
>> if we enable the IOMMU in perhaps a flawed way.
>>
>> A formally valid IVMD we can't make sense of is imo no different from
>> a formally invalid IVMD, for which we would return ENODEV as well (and
>> hence fail to enable the IOMMU). Whereas what you're suggesting is, if
>> I take it further, to basically ignore (almost) all errors in table
>> parsing, and enable the IOMMU(s) in a best effort manner, no matter
>> whether that leads to a functional (let alone secure [to the degree
>> possible]) system.
> 
> No, don't take it further: not ignore all errors, I think we should
> ignore errors when the device in the IVMD is not behind an IOMMU.  And
> I think that should apply globally, regardless of whether all devices
> in the IVMD block fall in that category.
> 
> That will bring AMD-Vi inline with VT-d RMRR, as from what I can see
> acpi_parse_one_rmrr() doesn't care whether the device scope in the
> entry is behind an IOMMU or not, or whether the whole RMRR doesn't
> effectively apply to any device because none of them is covered by an
> IOMMU.
> 
>> What I don't really understand is why you want to relax our checking
>> beyond what's necessary for the one issue at hand.
> 
> This issue has been reported to us and we have been able to debug.
> However, I worry what other malformed IVMD blocks might be out there,
> for example an IVMD block that applies to a single device (type 21h),
> but such single device doesn't exist (or it's not behind an IOMMU).
> Maybe next time we simply won't get a report, the user will try Xen,
> see it's not working and move to something else.
> 
> I've taken a quick look at Linux, and when parsing the IVMD blocks
> there's no checking that referred devices are behind an IOMMU, the
> regions are just added to a list.

Hmm, okay, after some more chewing on it on this basis
Acked-by: Jan Beulich 

Jan

Re: [PATCH v9 2/2] x86/boot: Improve MBI2 structure check

[Jan Beulich]

On 10.10.2024 11:45, Frediano Ziglio wrote:
> Tag structure should contain at least the tag header.
> Entire tag structure must be contained inside MBI2 data.
> 
> Reviewed-by: Jan Beulich 
> Signed-off-by: Frediano Ziglio 

Nit (for the future): Tags in chronological order please (maybe
with the exception of release acks, which personally I think best
go last).

Jan

Re: [PATCH v4 1/6] x86/boot: Prep work for 32bit object changes

[Jan Beulich]

On 14.10.2024 10:53, Frediano Ziglio wrote:
> Broken out of the subsequent patch for clarity.
> 
>  * Rename head-bin-objs to obj32
>  * Use a .32.o suffix to distinguish these objects
>  * Factor out $(LD32)
> 
> No functional change.
> 
> Signed-off-by: Frediano Ziglio 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

Re: [RFC QEMU PATCH v7 1/1] xen/pci: get gsi for passthrough devices

[Stewart Hildebrand]

+Edgar

On 5/16/24 06:13, Jiqian Chen wrote:
> In PVH dom0, it uses the linux local interrupt mechanism,
> when it allocs irq for a gsi, it is dynamic, and follow
> the principle of applying first, distributing first. And
> the irq number is alloced from small to large, but the
> applying gsi number is not, may gsi 38 comes before gsi
> 28, that causes the irq number is not equal with the gsi
> number. And when passthrough a device, qemu wants to use
> gsi to map pirq, xen_pt_realize->xc_physdev_map_pirq, but
> the gsi number is got from file
> /sys/bus/pci/devices//irq in current code, so it
> will fail when mapping.
> 
> Get gsi by using new function supported by Xen tools.
> 
> Signed-off-by: Huang Rui 
> Signed-off-by: Jiqian Chen 

I think you can safely remove the RFC tag since the Xen bits have been
upstreamed.

> ---
>  hw/xen/xen-host-pci-device.c | 19 +++
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716a2..2fe6a60434ba 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -10,6 +10,7 @@
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "xen-host-pci-device.h"
> +#include "hw/xen/xen_native.h"

The inclusion order unfortunately seems to be delicate.
"hw/xen/xen_native.h" should be before all the other xen
includes, but after "qemu/osdep.h".

>  
>  #define XEN_HOST_PCI_MAX_EXT_CAP \
>  ((PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE) / (PCI_CAP_SIZEOF + 4))
> @@ -329,12 +330,17 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice 
> *d, uint32_t cap)
>  return -1;
>  }
>  
> +#define PCI_SBDF(seg, bus, dev, func) \
> +uint32_t)(seg)) << 16) | \
> +(PCI_BUILD_BDF(bus, PCI_DEVFN(dev, func
> +
>  void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>   uint8_t bus, uint8_t dev, uint8_t func,
>   Error **errp)
>  {
>  ERRP_GUARD();
>  unsigned int v;
> +uint32_t sdbf;

Typo: s/sdbf/sbdf/

>  
>  d->config_fd = -1;
>  d->domain = domain;
> @@ -364,11 +370,16 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, 
> uint16_t domain,
>  }
>  d->device_id = v;
>  
> -xen_host_pci_get_dec_value(d, "irq", &v, errp);
> -if (*errp) {
> -goto error;
> +sdbf = PCI_SBDF(domain, bus, dev, func);
> +d->irq = xc_physdev_gsi_from_dev(xen_xc, sdbf);

This was renamed to xc_pcidev_get_gsi.

This also needs some sort of Xen interface version guard for backward
compatibility since it's a new call introduced in Xen 4.20.

> +/* fail to get gsi, fallback to irq */
> +if (d->irq == -1) {
> +xen_host_pci_get_dec_value(d, "irq", &v, errp);
> +if (*errp) {
> +goto error;
> +}
> +d->irq = v;
>  }
> -d->irq = v;
>  
>  xen_host_pci_get_hex_value(d, "class", &v, errp);
>  if (*errp) {

Re: Xen PAT settings vs Linux PAT settings

[Andrew Cooper]

On 14/10/2024 7:26 pm, Marek Marczykowski-Górecki wrote:
> Hi,
>
> It looks like we've identified the second buggy driver that somewhere
> assumes PAT is configured as Linux normally do natively - nvidia binary
> one this time[3]. The first one affected was i915, but it turned out to be
> a bug in Linux mm. It was eventually fixed[1], but it was quite painful
> debugging. This time a proper fix is not known yet. Since the previous
> issue, Qubes OS carried a patch[2] that changes Xen to use same PAT as
> Linux. We recently dropped this patch, since the Linux fix reached all
> supported by us branches, but apparently it wasn't all...
>
> Anyway, would it be useful (and acceptable) for upstream Xen to have
> a kconfig option (behind UNSUPPORTED or so) to switch this behavior?

Not UNSUPPORTED - it's bogus and I still want it purged.

But, behind EXPERT, with a suitable description (e.g. "This breaks
various ABIs including migration, and is presented here for debugging PV
driver issues in a single system.  If turning it on fixes a bug, please
contact upstream Xen"), then I think we need to take it.

The fact that I've had to recommend it once already this week for
debugging purposes, and it wasn't even this Nvidia bug, demonstrates how
pervasive the problems are.

> Technically, it's a PV ABI violation, and it does break few things
> (definitely PV domU with passthrough are affected - Xen considers them
> L1TF vulnerable then; PV live migration is most likely broken too).

Do you have more information on this?  The PAT bits shouldn't form any
part of L1TF considerations.

~Andrew

Xen PAT settings vs Linux PAT settings

[Marek Marczykowski-Górecki]

Hi,

It looks like we've identified the second buggy driver that somewhere
assumes PAT is configured as Linux normally do natively - nvidia binary
one this time[3]. The first one affected was i915, but it turned out to be
a bug in Linux mm. It was eventually fixed[1], but it was quite painful
debugging. This time a proper fix is not known yet. Since the previous
issue, Qubes OS carried a patch[2] that changes Xen to use same PAT as
Linux. We recently dropped this patch, since the Linux fix reached all
supported by us branches, but apparently it wasn't all...

Anyway, would it be useful (and acceptable) for upstream Xen to have
a kconfig option (behind UNSUPPORTED or so) to switch this behavior?
Technically, it's a PV ABI violation, and it does break few things
(definitely PV domU with passthrough are affected - Xen considers them
L1TF vulnerable then; PV live migration is most likely broken too). But
on the other hand, if one doesn't use affected feature, it allows to
workaround an issue that otherwise is very annoying to debug...


[1] git.kernel.org/torvalds/c/548cb932051fb6232ac983ed6673dae7bdf3cf4c
[2] 
https://github.com/QubesOS/qubes-vmm-xen/blob/44e9fd9f3b1ebf1cf43674b5a1c2669f7dd253f5/1019-Use-Linux-s-PAT.patch
[3] https://github.com/QubesOS/qubes-issues/issues/9501
-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab


signature.asc
Description: PGP signature

Re: Xen PAT settings vs Linux PAT settings

[Andrew Cooper]

On 14/10/2024 10:37 pm, Marek Marczykowski-Górecki wrote:
> On Mon, Oct 14, 2024 at 09:05:58PM +0100, Andrew Cooper wrote:
>> On 14/10/2024 7:26 pm, Marek Marczykowski-Górecki wrote:
>>> Technically, it's a PV ABI violation, and it does break few things
>>> (definitely PV domU with passthrough are affected - Xen considers them
>>> L1TF vulnerable then; PV live migration is most likely broken too).
>> Do you have more information on this?  The PAT bits shouldn't form any
>> part of L1TF considerations.
> https://github.com/QubesOS/qubes-issues/issues/8593
>

0x801018200066

That's a very L1TF-unsafe PTE, but it's also got nothing to do with PAT.
It's:

  NX | Avail(bit 52) | addr (0x1820) | D | A | U | W

and importantly not present.  PAT == 0 == WB in both the Xen and Linux
worlds.

But, it likely does highlight a codepath which is opencoding PTE updates.

We really ought to have an option to do as f61c54967f4a did with
_PAGE_GNTTAB, and to inject #GP into the guest to get a backtrace out of
Linux.

In the case that we're going to crash the domain anyway, #GP is still
more useful, although I would quite like the #GP option instead of
shadowing too.  Maybe hanging off pv-l1tf=fault as an option?

~Andrew

[xen-unstable test] 188085: tolerable FAIL - PUSHED

[osstest service owner]

flight 188085 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/188085/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 188079
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 188079
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 188079
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 188079
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 188079
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 188079
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  542ac112fc68c66cfafc577e252404c21da4f75b
baseline version:
 xen  76a54badf890f56ff72644593c0fbc72138e13aa

Last test of basis   188079  2024-10-14 01:54:05 Z1 days
Testing same since   188085  2024-10-14 19:09:00 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt

Re: [PATCH v5 1/3] x86/msi: harden stale pdev handling

[Jan Beulich]

On 11.10.2024 17:27, Stewart Hildebrand wrote:
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1243,7 +1243,12 @@ int pci_reset_msix_state(struct pci_dev *pdev)
>  {
>  unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
>  
> -ASSERT(pos);
> +if ( !pos )
> +{
> +pdev->broken = true;
> +return -EFAULT;
> +}
> +
>  /*
>   * Xen expects the device state to be the after reset one, and hence
>   * host_maskall = guest_maskall = false and all entries should have the
> @@ -1271,7 +1276,12 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, 
> unsigned int reg,
>  entry = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
>  pos = entry ? entry->msi_attrib.pos
>  : pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
> -ASSERT(pos);
> +
> +if ( !pos )
> +{
> +pdev->broken = true;
> +return -EFAULT;
> +}
>  
>  if ( reg >= pos && reg < msix_pba_offset_reg(pos) + 4 )
>  {

There are more instances of pci_find_cap_offset(..., PCI_CAP_ID_MSIX)
which may want/need dealing with, even if there are no ASSERT()s there.

Setting ->broken is of course a perhaps desirable (side) effect. Nevertheless
I wonder whether latching the capability position once during device init
wouldn't be an alternative (better?) approach.

Finally I don't think -EFAULT is appropriate here. Imo it should be -ENODEV.

Jan

Re: [PATCH] xen/public: add comments regarding interface version bumps

[Jan Beulich]

On 14.10.2024 12:33, Juergen Gross wrote:
> domctl.h and sysctl.h have an interface version, which needs to be
> bumped in case of incompatible modifications of the interface.
> 
> In order to avoid misunderstandings, add a comment to both headers
> specifying in which cases a bump is needed.
> 
> Signed-off-by: Juergen Gross 

Reviewed-by: Jan Beulich 

Re: [PATCH] xen/public: add comments regarding interface version bumps

[Julien Grall]

Hi Juergen,

On 14/10/2024 11:33, Juergen Gross wrote:

domctl.h and sysctl.h have an interface version, which needs to be
bumped in case of incompatible modifications of the interface.


What about vm_event.h?



In order to avoid misunderstandings, add a comment to both headers
specifying in which cases a bump is needed.

Signed-off-by: Juergen Gross 


Regardless my question above:

Acked-by: Julien Grall 

Cheers,

--
Julien Grall

Re: [PATCH] xen/public: increment domctl interface version

[Jan Beulich]

On 14.10.2024 10:00, Jürgen Groß wrote:
> On 14.10.24 09:46, Jan Beulich wrote:
>> On 14.10.2024 09:36, Jürgen Groß wrote:
>>> On 14.10.24 09:14, Jan Beulich wrote:
 On 14.10.2024 09:06, Juergen Gross wrote:
> The recent addition of the XEN_DOMCTL_dt_overlay function was missing
> the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
> first interface change of the 4.20 release cycle.
>
> Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device 
> attachment to domains")

 I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
 bumped the version ...

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
>#include "hvm/save.h"
>#include "memory.h"
>
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x0018

 ... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
 require a bump. Did you maybe mean to reference a different commit?
>>>
>>> Oh, indeed. I wanted to reference d6e9a2aab39e.
>>>
>>> And regarding to "plain additions don't require a bump": 4c733873b5c2 did
>>> a plain addition and bumped the version.
>>
>> Right, hence why I said "for no apparent reason".
> 
> There seems to be a lack of documentation in this regard.
> 
> Julien explicitly asked for the bump for that addition.

Julien - why was that? Bumps are needed only for backwards incompatible
changes. Plain additions therefore never require a bump. As as we get
better with properly checking e.g. padding fields, the frequency of
required bumps should also further reduce.

> I'm fine with dropping my patch if others agree that the bump isn't needed.
> In that case I'll send another one adding a comment for the mechanics of
> interface version bump in domctl.h and sysctl.h.

Oh, yes, please feel free to do so.

Jan

[PATCH] xen/public: add comments regarding interface version bumps

[Juergen Gross]

domctl.h and sysctl.h have an interface version, which needs to be
bumped in case of incompatible modifications of the interface.

In order to avoid misunderstandings, add a comment to both headers
specifying in which cases a bump is needed.

Signed-off-by: Juergen Gross 
---
 xen/include/public/domctl.h | 10 ++
 xen/include/public/sysctl.h | 10 ++
 2 files changed, 20 insertions(+)

diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e1028fc524..353f831e40 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,6 +21,16 @@
 #include "hvm/save.h"
 #include "memory.h"
 
+/*
+ * The interface version needs to be incremented by 1 in case the interface
+ * is modified in an incompatible way AND if the version hasn't been
+ * incremented in the current development cycle already.
+ * Pure additions (e.g. new sub-commands) or compatible interface changes
+ * (e.g. adding semantics to 0-checked input fields or data to zeroed output
+ * fields) don't require a change of the version.
+ *
+ * Last version bump: Xen 4.19
+ */
 #define XEN_DOMCTL_INTERFACE_VERSION 0x0017
 
 /*
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index b2a5a724db..b0fec271d3 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -18,6 +18,16 @@
 #include "domctl.h"
 #include "physdev.h"
 
+/*
+ * The interface version needs to be incremented by 1 in case the interface
+ * is modified in an incompatible way AND if the version hasn't been
+ * incremented in the current development cycle already.
+ * Pure additions (e.g. new sub-commands) or compatible interface changes
+ * (e.g. adding semantics to 0-checked input fields or data to zeroed output
+ * fields) don't require a change of the version.
+ *
+ * Last version bump: Xen 4.17
+ */
 #define XEN_SYSCTL_INTERFACE_VERSION 0x0015
 
 /*
-- 
2.43.0

Re: [PATCH] xen/public: increment domctl interface version

[Julien Grall]

Hi Jan,

On 14/10/2024 11:19, Jan Beulich wrote:

On 14.10.2024 10:00, Jürgen Groß wrote:

On 14.10.24 09:46, Jan Beulich wrote:

On 14.10.2024 09:36, Jürgen Groß wrote:

On 14.10.24 09:14, Jan Beulich wrote:

On 14.10.2024 09:06, Juergen Gross wrote:

The recent addition of the XEN_DOMCTL_dt_overlay function was missing
the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
first interface change of the 4.20 release cycle.

Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to 
domains")


I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
bumped the version ...


--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
#include "hvm/save.h"
#include "memory.h"

-#define XEN_DOMCTL_INTERFACE_VERSION 0x0017

+#define XEN_DOMCTL_INTERFACE_VERSION 0x0018


... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
require a bump. Did you maybe mean to reference a different commit?


Oh, indeed. I wanted to reference d6e9a2aab39e.

And regarding to "plain additions don't require a bump": 4c733873b5c2 did
a plain addition and bumped the version.


Right, hence why I said "for no apparent reason".


There seems to be a lack of documentation in this regard.

Julien explicitly asked for the bump for that addition.


Julien - why was that? 


I can't exactly remember why... I possibly just assumed that we updated 
the version every release...


Cheers,

--
Julien Grall

Re: [PATCH] xen/public: add comments regarding interface version bumps

[Jürgen Groß]

On 14.10.24 12:48, Julien Grall wrote:

Hi Juergen,

On 14/10/2024 11:33, Juergen Gross wrote:

domctl.h and sysctl.h have an interface version, which needs to be
bumped in case of incompatible modifications of the interface.


What about vm_event.h?


Indeed.

There seem to be others missing, too. Here a git grep output of
possible candidates (including domctl.h, sysctl.h and vm_event.h):

arch-x86/xen-mca.h:#define XEN_MCA_INTERFACE_VERSION 0x01ecc003
domctl.h:#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
hvm/hvm_op.h:#define HVMOP_ALTP2M_INTERFACE_VERSION 0x0001
platform.h:#define XENPF_INTERFACE_VERSION 0x0301
sysctl.h:#define XEN_SYSCTL_INTERFACE_VERSION 0x0015
vm_event.h:#define VM_EVENT_INTERFACE_VERSION 0x0007
xsm/flask_op.h:#define XEN_FLASK_INTERFACE_VERSION 1

I can add another patch for the missing ones (or 1 patch for each?)





In order to avoid misunderstandings, add a comment to both headers
specifying in which cases a bump is needed.

Signed-off-by: Juergen Gross 


Regardless my question above:

Acked-by: Julien Grall 


Thanks,

Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

[RFC PATCH v1 36/57] xen: Remove PAGE_SIZE compile-time constant assumption

[Ryan Roberts]

To prepare for supporting boot-time page size selection, refactor code
to remove assumptions about PAGE_SIZE being compile-time constant. Code
intended to be equivalent when compile-time page size is active.

Allocate enough "frame_list" static storage in the balloon driver for
the maximum supported page size. Although continue to use only the first
PAGE_SIZE of the buffer at run-time to maintain existing behaviour.

Refactor xen_biovec_phys_mergeable() to convert ifdeffery to c if/else.
For compile-time page size, the compiler will choose one branch and
strip the dead one. For boot-time, it can be evaluated at run time.

Refactor a BUILD_BUG_ON to evaluate the limit (when the minimum
supported page size is selected at boot-time).

Reserve enough storage for max page size in "struct remap_data" and
"struct xenbus_map_node".

Signed-off-by: Ryan Roberts 
---

***NOTE***
Any confused maintainers may want to read the cover note here for context:
https://lore.kernel.org/all/20241014105514.3206191-1-ryan.robe...@arm.com/

 drivers/xen/balloon.c  | 11 ++-
 drivers/xen/biomerge.c | 12 ++--
 drivers/xen/privcmd.c  |  2 +-
 drivers/xen/xenbus/xenbus_client.c |  5 +++--
 drivers/xen/xlate_mmu.c|  6 +++---
 include/xen/page.h |  2 ++
 6 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 528395133b4f8..0ed5f6453af0e 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -131,7 +131,8 @@ struct balloon_stats balloon_stats;
 EXPORT_SYMBOL_GPL(balloon_stats);
 
 /* We increase/decrease in batches which fit in a page */
-static xen_pfn_t frame_list[PAGE_SIZE / sizeof(xen_pfn_t)];
+static xen_pfn_t frame_list[PAGE_SIZE_MAX / sizeof(xen_pfn_t)];
+#define FRAME_LIST_NR_ENTRIES (PAGE_SIZE / sizeof(xen_pfn_t))
 
 
 /* List of ballooned pages, threaded through the mem_map array. */
@@ -389,8 +390,8 @@ static enum bp_state increase_reservation(unsigned long 
nr_pages)
unsigned long i;
struct page   *page;
 
-   if (nr_pages > ARRAY_SIZE(frame_list))
-   nr_pages = ARRAY_SIZE(frame_list);
+   if (nr_pages > FRAME_LIST_NR_ENTRIES)
+   nr_pages = FRAME_LIST_NR_ENTRIES;
 
page = list_first_entry_or_null(&ballooned_pages, struct page, lru);
for (i = 0; i < nr_pages; i++) {
@@ -434,8 +435,8 @@ static enum bp_state decrease_reservation(unsigned long 
nr_pages, gfp_t gfp)
int ret;
LIST_HEAD(pages);
 
-   if (nr_pages > ARRAY_SIZE(frame_list))
-   nr_pages = ARRAY_SIZE(frame_list);
+   if (nr_pages > FRAME_LIST_NR_ENTRIES)
+   nr_pages = FRAME_LIST_NR_ENTRIES;
 
for (i = 0; i < nr_pages; i++) {
page = alloc_page(gfp);
diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c
index 05a286d24f148..28f0887e40026 100644
--- a/drivers/xen/biomerge.c
+++ b/drivers/xen/biomerge.c
@@ -8,16 +8,16 @@
 bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
   const struct page *page)
 {
-#if XEN_PAGE_SIZE == PAGE_SIZE
-   unsigned long bfn1 = pfn_to_bfn(page_to_pfn(vec1->bv_page));
-   unsigned long bfn2 = pfn_to_bfn(page_to_pfn(page));
+   if (XEN_PAGE_SIZE == PAGE_SIZE) {
+   unsigned long bfn1 = pfn_to_bfn(page_to_pfn(vec1->bv_page));
+   unsigned long bfn2 = pfn_to_bfn(page_to_pfn(page));
+
+   return bfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == bfn2;
+   }
 
-   return bfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == bfn2;
-#else
/*
 * XXX: Add support for merging bio_vec when using different page
 * size in Xen and Linux.
 */
return false;
-#endif
 }
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 9563650dfbafc..847f7b806caf7 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -557,7 +557,7 @@ static long privcmd_ioctl_mmap_batch(
state.global_error  = 0;
state.version   = version;
 
-   BUILD_BUG_ON(((PAGE_SIZE / sizeof(xen_pfn_t)) % XEN_PFN_PER_PAGE) != 0);
+   BUILD_BUG_ON(((PAGE_SIZE_MIN / sizeof(xen_pfn_t)) % 
XEN_PFN_PER_PAGE_MAX) != 0);
/* mmap_batch_fn guarantees ret == 0 */
BUG_ON(traverse_pages_block(m.num, sizeof(xen_pfn_t),
&pagelist, mmap_batch_fn, &state));
diff --git a/drivers/xen/xenbus/xenbus_client.c 
b/drivers/xen/xenbus/xenbus_client.c
index 51b3124b0d56c..99bde836c10c4 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -49,9 +49,10 @@
 
 #include "xenbus.h"
 
-#define XENBUS_PAGES(_grants)  (DIV_ROUND_UP(_grants, XEN_PFN_PER_PAGE))
+#define XENBUS_PAGES(_grants)  (DIV_ROUND_UP(_grants, 
XEN_PFN_PER_PAGE))
+#define XENBUS_PAGES_MAX(_grants)  (DIV_ROUND_UP(_grants, 
XEN_PFN_PER_PAGE_MIN))
 
-#define XENBUS_MAX_RING_PAGES  (XENBUS_PAGES(XENBU

Re: [PATCH] xen/public: increment domctl interface version

[Jürgen Groß]

On 14.10.24 09:46, Jan Beulich wrote:

On 14.10.2024 09:36, Jürgen Groß wrote:

On 14.10.24 09:14, Jan Beulich wrote:

On 14.10.2024 09:06, Juergen Gross wrote:

The recent addition of the XEN_DOMCTL_dt_overlay function was missing
the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
first interface change of the 4.20 release cycle.

Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to 
domains")


I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
bumped the version ...


--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
   #include "hvm/save.h"
   #include "memory.h"
   
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0017

+#define XEN_DOMCTL_INTERFACE_VERSION 0x0018


... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
require a bump. Did you maybe mean to reference a different commit?


Oh, indeed. I wanted to reference d6e9a2aab39e.

And regarding to "plain additions don't require a bump": 4c733873b5c2 did
a plain addition and bumped the version.


Right, hence why I said "for no apparent reason".


There seems to be a lack of documentation in this regard.

Julien explicitly asked for the bump for that addition.

I'm fine with dropping my patch if others agree that the bump isn't needed.
In that case I'll send another one adding a comment for the mechanics of
interface version bump in domctl.h and sysctl.h.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH] xen/public: add comments regarding interface version bumps

[Jan Beulich]

On 14.10.2024 13:01, Jürgen Groß wrote:
> On 14.10.24 12:48, Julien Grall wrote:
>> On 14/10/2024 11:33, Juergen Gross wrote:
>>> domctl.h and sysctl.h have an interface version, which needs to be
>>> bumped in case of incompatible modifications of the interface.
>>
>> What about vm_event.h?
> 
> Indeed.
> 
> There seem to be others missing, too. Here a git grep output of
> possible candidates (including domctl.h, sysctl.h and vm_event.h):
> 
> arch-x86/xen-mca.h:#define XEN_MCA_INTERFACE_VERSION 0x01ecc003
> domctl.h:#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
> hvm/hvm_op.h:#define HVMOP_ALTP2M_INTERFACE_VERSION 0x0001
> platform.h:#define XENPF_INTERFACE_VERSION 0x0301
> sysctl.h:#define XEN_SYSCTL_INTERFACE_VERSION 0x0015
> vm_event.h:#define VM_EVENT_INTERFACE_VERSION 0x0007
> xsm/flask_op.h:#define XEN_FLASK_INTERFACE_VERSION 1
> 
> I can add another patch for the missing ones (or 1 patch for each?)

Please don't. The situation is different there. Sysctl and domctl are
unstable interfaces, hence to change them in incompatible ways a bump
is all that's needed (in addition). That's quite different from cases
where the interface itself is stable. The only possible exception to
this is vm_event.h, which offers solely tools-only interfaces. I'm
not sure though whether "just" bumping the interface version there is
enough for an incompatible change to occur. Tamas?

Jan

Re: [PATCH] xen/public: increment domctl interface version

[Jürgen Groß]

On 14.10.24 09:14, Jan Beulich wrote:

On 14.10.2024 09:06, Juergen Gross wrote:

The recent addition of the XEN_DOMCTL_dt_overlay function was missing
the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
first interface change of the 4.20 release cycle.

Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to 
domains")


I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
bumped the version ...


--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
  #include "hvm/save.h"
  #include "memory.h"
  
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0017

+#define XEN_DOMCTL_INTERFACE_VERSION 0x0018


... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
require a bump. Didi you maybe mean to reference a different commit?


Oh, indeed. I wanted to reference d6e9a2aab39e.

And regarding to "plain additions don't require a bump": 4c733873b5c2 did
a plain addition and bumped the version.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

[PATCH v4 4/6] x86/boot: Use boot_vid_info variable directly from C code

[Frediano Ziglio]

No more need to pass from assembly code.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Andrew Cooper 
---
Changes since v1:
- split the 2 variable changes into 2 commits.

Changes since v2:
- revert commit order.
---
 xen/arch/x86/boot/build32.lds.S |  1 +
 xen/arch/x86/boot/head.S| 10 +-
 xen/arch/x86/boot/reloc.c   | 13 +
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 1110880ad4..8c40758834 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -46,6 +46,7 @@ SECTIONS
 DECLARE_IMPORT(__trampoline_seg_start);
 DECLARE_IMPORT(__trampoline_seg_stop);
 DECLARE_IMPORT(trampoline_phys);
+DECLARE_IMPORT(boot_vid_info);
 . = . + GAP;
 *(.text)
 *(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ade2c5c43d..5da7ac138f 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -514,18 +514,10 @@ trampoline_setup:
 mov sym_esi(trampoline_phys), %ecx
 add $TRAMPOLINE_SPACE,%ecx
 
-#ifdef CONFIG_VIDEO
-lea sym_esi(boot_vid_info), %edx
-#else
-xor %edx, %edx
-#endif
-
 /* Save Multiboot / PVH info struct (after relocation) for later use. 
*/
-push%edx/* Boot video info to be filled from MB2. 
*/
 mov %ebx, %edx  /* Multiboot / PVH information address. */
-/*  reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using 
fastcall. */
+/*  reloc(magic/eax, info/edx, trampoline/ecx) using fastcall. */
 callreloc
-add $4, %esp
 
 #ifdef CONFIG_PVH_GUEST
 cmpb$0, sym_esi(pvh_boot)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 94b078d7b1..707d9c5f15 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -176,7 +176,7 @@ static multiboot_info_t *mbi_reloc(uint32_t mbi_in, memctx 
*ctx)
 return mbi_out;
 }
 
-static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, 
memctx *ctx)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
 {
 const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
 const multiboot2_memory_map_t *mmap_src;
@@ -185,7 +185,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out, memctx
 memory_map_t *mmap_dst;
 multiboot_info_t *mbi_out;
 #ifdef CONFIG_VIDEO
-struct boot_video_info *video = NULL;
+struct boot_video_info *video = &boot_vid_info;
 #endif
 uint32_t ptr;
 unsigned int i, mod_idx = 0;
@@ -290,12 +290,11 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out, memctx
 
 #ifdef CONFIG_VIDEO
 case MULTIBOOT2_TAG_TYPE_VBE:
-if ( video_out )
+if ( video )
 {
 const struct vesa_ctrl_info *ci;
 const struct vesa_mode_info *mi;
 
-video = _p(video_out);
 ci = (const void *)get_mb2_data(tag, vbe, vbe_control_info);
 mi = (const void *)get_mb2_data(tag, vbe, vbe_mode_info);
 
@@ -321,7 +320,6 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out, memctx
 if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
   MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
 {
-video_out = 0;
 video = NULL;
 }
 break;
@@ -346,8 +344,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out, memctx
 }
 
 /* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
-uint32_t video_info)
+void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline)
 {
 memctx ctx = { trampoline };
 
@@ -357,7 +354,7 @@ void *reloc(uint32_t magic, uint32_t in, uint32_t 
trampoline,
 return mbi_reloc(in, &ctx);
 
 case MULTIBOOT2_BOOTLOADER_MAGIC:
-return mbi2_reloc(in, video_info, &ctx);
+return mbi2_reloc(in, &ctx);
 
 case XEN_HVM_START_MAGIC_VALUE:
 if ( IS_ENABLED(CONFIG_PVH_GUEST) )
-- 
2.34.1

[PATCH v4 5/6] x86/boot: Use trampoline_phys variable directly from C code

[Frediano Ziglio]

No more need to pass from assembly code.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Andrew Cooper 
---
Changes since v1:
- split the 2 variable changes into 2 commits.

Changes since v2:
- revert commit order;
- avoid useless casts.
---
 xen/arch/x86/boot/head.S  | 6 +-
 xen/arch/x86/boot/reloc.c | 8 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 5da7ac138f..dcda91cfda 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -510,13 +510,9 @@ trampoline_setup:
 mov %esi, sym_esi(xen_phys_start)
 mov %esi, sym_esi(trampoline_xen_phys_start)
 
-/* Get bottom-most low-memory stack address. */
-mov sym_esi(trampoline_phys), %ecx
-add $TRAMPOLINE_SPACE,%ecx
-
 /* Save Multiboot / PVH info struct (after relocation) for later use. 
*/
 mov %ebx, %edx  /* Multiboot / PVH information address. */
-/*  reloc(magic/eax, info/edx, trampoline/ecx) using fastcall. */
+/*  reloc(magic/eax, info/edx) using fastcall. */
 callreloc
 
 #ifdef CONFIG_PVH_GUEST
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 707d9c5f15..e50e161b27 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -19,6 +19,9 @@
 #include 
 #include 
 #include 
+#include 
+
+#include 
 
 #include 
 
@@ -344,9 +347,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
memctx *ctx)
 }
 
 /* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline)
+void *reloc(uint32_t magic, uint32_t in)
 {
-memctx ctx = { trampoline };
+/* Get bottom-most low-memory stack address. */
+memctx ctx = { trampoline_phys + TRAMPOLINE_SPACE };
 
 switch ( magic )
 {
-- 
2.34.1

[PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Frediano Ziglio]

The current method to include 32 bit C boot code is:
- compile each function we want to use into a separate object file;
- each function is compiled with -fpic option;
- convert these object files to binary files. This operation removes GOP
  which we don't want in the executable;
- a small assembly part in each file add the entry point;
- code can't have external references, all possible variables are passed
  by value or pointer;
- include these binary files in head.S.

There are currently some limitations:
- code is compiled separately, it's not possible to share a function
  (like memcpy) between different functions to use;
- although code is compiled with -fpic there's no certainty there are
  no relocations, specifically data ones. This can lead into hard to
  find bugs;
- it's hard to add a simple function;
- having to pass external variables makes hard to do multiple things
  otherwise functions would require a lot of parameters so code would
  have to be split into multiple functions which is not easy.

Current change extends the current process:
- all object files are linked together before getting converted making
  possible to share code between the function we want to call;
- a single object file is generated with all functions to use and
  exported symbols to easily call;
- variables to use are declared in linker script and easily used inside
  C code. Declaring them manually could be annoying but makes also
  easier to check them. Using external pointers can be still an issue if
  they are not fixed. If an external symbol is not declared this gives a
  link error.

Some details of the implementation:
- C code is compiled with -fpic flags (as before);
- object files from C code are linked together;
- the single bundled object file is linked with 2 slightly different
  script files to generate 2 bundled object files;
- the 2 bundled object files are converted to binary removing the need
  for global offset tables;
- a Python script is used to generate assembly source from the 2
  binaries;
- the single assembly file is compiled to generate final bundled object
  file;
- to detect possible unwanted relocation in data/code code is generated
  with different addresses. This is enforced starting .text section at
  different positions and adding a fixed "gap" at the beginning.
  This makes sure code and data is position independent;
- to detect used symbols in data/code symbols are placed in .text
  section at different offsets (based on the line in the linker script).
  This is needed as potentially a reference to a symbol is converted to
  a reference to the containing section so multiple symbols could be
  converted to reference to same symbol (section name) and we need to
  distinguish them;
- --orphan-handling=error option to linker is used to make sure we
  account for all possible sections from C code;

Current limitations:
- the main one is the lack of support for 64 bit code. It would make
  sure that even the code used for 64 bit (at the moment EFI code) is
  code and data position independent. We cannot assume that code that
  came from code compiled for 32 bit and compiled for 64 bit is code and
  data position independent, different compiler options lead to
  different code/data.

Signed-off-by: Frediano Ziglio 
---
Changes since v2:
- removed W^X limitation, allowing data;
- added some comments to python script;
- added extension to python script;
- added header to generated assembly code from python script;
- added starting symbol to generated assembly code from python script
  to make disassembly more clear;
- other minor style changes to python script.
---
 xen/arch/x86/boot/.gitignore  |   5 +-
 xen/arch/x86/boot/Makefile|  38 +++-
 .../x86/boot/{build32.lds => build32.lds.S}   |  35 ++-
 xen/arch/x86/boot/cmdline.c   |  12 -
 xen/arch/x86/boot/head.S  |  12 -
 xen/arch/x86/boot/reloc.c |  14 --
 xen/tools/combine_two_binaries.py | 207 ++
 7 files changed, 268 insertions(+), 55 deletions(-)
 rename xen/arch/x86/boot/{build32.lds => build32.lds.S} (70%)
 create mode 100755 xen/tools/combine_two_binaries.py

diff --git a/xen/arch/x86/boot/.gitignore b/xen/arch/x86/boot/.gitignore
index a379db7988..ebad650e5c 100644
--- a/xen/arch/x86/boot/.gitignore
+++ b/xen/arch/x86/boot/.gitignore
@@ -1,3 +1,4 @@
 /mkelf32
-/*.bin
-/*.lnk
+/build32.*.lds
+/built_in_32.*.bin
+/built_in_32.*.map
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 1199291d2b..23ad274c89 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,4 +1,5 @@
 obj-bin-y += head.o
+obj-bin-y += built_in_32.o
 
 obj32 := cmdline.32.o
 obj32 += reloc.32.o
@@ -9,9 +10,6 @@ targets   += $(obj32)
 
 obj32 := $(addprefix $(obj)/,$(obj32))
 
-$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(obj32:.32.o=.bin)
-
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=

[PATCH v4 0/6] Reuse 32 bit C code more safely

[Frediano Ziglio]

This series attempt to:
- use more C code, that is replace some assembly code with C;
- avoid some code duplication between C and assembly;
- prevent some issues having relocations in C code.

The idea is extending the current C to binary code conversion
done for 32 bit C code called from head.S making sure relocations
are safe and allowing external symbols usage from C code.

More details of the implementation are in commit message 2/6,
which is the largest patch.
Patch 3/6 reuses code to relocate the trampoline between 32 and 64 bit.
Patches 4/6 and 5/6 move some code from assembly to C.

Other RFC commits were excluded, I didn't manage to entangle the issues
sharing headers between 32 and 64 bits.
Since RFC code was more tested, also with CI and some incompatibility
were fixed. On that it's weird that we need Python 3.8 for Qemu but we
still use Python 2 for Xen. Shouldn't we bump requirement to Python 3
even for Xen?

Code boots successfully using:
- BIOS boot;
- EFI boot with Grub2 and ELF file;
- direct EFI boot without Grub.

Code is currently based on "master" branch, currently commit
76a54badf890f56ff72644593c0fbc72138e13aa.

Changes since v1:
- 2 preliminary commits for adjust .gitignore;
- last commit split (one variable at a time);
- lot of style and names changes;
- first commit, now 3/6 had some build changes, more details on
  commit message.

Changes since v2:
- removed merged commits;
- reverted order of 2 commits;
- remove some useless casts;
- added change to comment.

Changes since v3:
- added a preparation commit for Makefiles (mainly written by Andrew Cooper);
- added a comment improvement commit;
- allows also data;
- other minor style changes;
- added some Reviewed-by.

Frediano Ziglio (6):
  x86/boot: Prep work for 32bit object changes
  x86/boot: create a C bundle for 32 bit boot code and use it
  x86/boot: Reuse code to relocate trampoline
  x86/boot: Use boot_vid_info variable directly from C code
  x86/boot: Use trampoline_phys variable directly from C code
  x86/boot: Clarify comment

 xen/arch/x86/boot/.gitignore  |   5 +-
 xen/arch/x86/boot/Makefile|  61 --
 .../x86/boot/{build32.lds => build32.lds.S}   |  41 +++-
 xen/arch/x86/boot/cmdline.c   |  12 -
 xen/arch/x86/boot/head.S  |  49 +
 xen/arch/x86/boot/reloc-trampoline.c  |  36 +++
 xen/arch/x86/boot/reloc.c |  35 +--
 xen/arch/x86/efi/efi-boot.h   |  15 +-
 xen/tools/combine_two_binaries.py | 207 ++
 9 files changed, 341 insertions(+), 120 deletions(-)
 rename xen/arch/x86/boot/{build32.lds => build32.lds.S} (63%)
 create mode 100644 xen/arch/x86/boot/reloc-trampoline.c
 create mode 100755 xen/tools/combine_two_binaries.py

-- 
2.34.1

[PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline

[Frediano Ziglio]

Move code from efi-boot.h to a separate, new, reloc-trampoline.c file.
Reuse this new code, compiling it for 32bit as well, to replace assembly
code in head.S doing the same thing.

Signed-off-by: Frediano Ziglio 
Reviewed-by: Andrew Cooper 
---
Changes since v3:
- fixed a typo in comment;
- added Reviewed-by.
---
 xen/arch/x86/boot/Makefile   | 12 ++
 xen/arch/x86/boot/build32.lds.S  |  5 
 xen/arch/x86/boot/head.S | 23 +-
 xen/arch/x86/boot/reloc-trampoline.c | 36 
 xen/arch/x86/efi/efi-boot.h  | 15 ++--
 5 files changed, 52 insertions(+), 39 deletions(-)
 create mode 100644 xen/arch/x86/boot/reloc-trampoline.c

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 23ad274c89..ca258a9729 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,12 +1,16 @@
 obj-bin-y += head.o
 obj-bin-y += built_in_32.o
+obj-bin-y += $(obj64)
 
 obj32 := cmdline.32.o
 obj32 += reloc.32.o
+obj32 += reloc-trampoline.32.o
 
-nocov-y   += $(obj32)
-noubsan-y += $(obj32)
-targets   += $(obj32)
+obj64 := reloc-trampoline.o
+
+nocov-y   += $(obj32) $(obj64)
+noubsan-y += $(obj32) $(obj64)
+targets   += $(obj32) $(obj64)
 
 obj32 := $(addprefix $(obj)/,$(obj32))
 
@@ -50,7 +54,7 @@ $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin 
$(obj)/built_in_32.final.bin
--bin1 $(obj)/built_in_32.other.bin \
--bin2 $(obj)/built_in_32.final.bin \
--map $(obj)/built_in_32.final.map \
-   --exports cmdline_parse_early,reloc \
+   --exports cmdline_parse_early,reloc,reloc_trampoline32 \
--output $@
 
 clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 50c167aef0..1110880ad4 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -41,6 +41,11 @@ SECTIONS
  * Potentially they should be all variables. */
 DECLARE_IMPORT(__base_relocs_start);
 DECLARE_IMPORT(__base_relocs_end);
+DECLARE_IMPORT(__trampoline_rel_start);
+DECLARE_IMPORT(__trampoline_rel_stop);
+DECLARE_IMPORT(__trampoline_seg_start);
+DECLARE_IMPORT(__trampoline_seg_stop);
+DECLARE_IMPORT(trampoline_phys);
 . = . + GAP;
 *(.text)
 *(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index e0776e3896..ade2c5c43d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -706,28 +706,7 @@ trampoline_setup:
 mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
 
 /* Apply relocations to bootstrap trampoline. */
-mov sym_esi(trampoline_phys), %edx
-lea sym_esi(__trampoline_rel_start), %edi
-lea sym_esi(__trampoline_rel_stop), %ecx
-1:
-mov (%edi), %eax
-add %edx, (%edi, %eax)
-add $4,%edi
-
-cmp %ecx, %edi
-jb  1b
-
-/* Patch in the trampoline segment. */
-shr $4,%edx
-lea sym_esi(__trampoline_seg_start), %edi
-lea sym_esi(__trampoline_seg_stop), %ecx
-1:
-mov (%edi), %eax
-mov %dx, (%edi, %eax)
-add $4,%edi
-
-cmp %ecx, %edi
-jb  1b
+callreloc_trampoline32
 
 /* Do not parse command line on EFI platform here. */
 cmpb$0, sym_esi(efi_platform)
diff --git a/xen/arch/x86/boot/reloc-trampoline.c 
b/xen/arch/x86/boot/reloc-trampoline.c
new file mode 100644
index 00..0a74c1e75a
--- /dev/null
+++ b/xen/arch/x86/boot/reloc-trampoline.c
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include 
+#include 
+#include 
+
+extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
+extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
+
+#if defined(__i386__)
+void reloc_trampoline32(void)
+#elif defined (__x86_64__)
+void reloc_trampoline64(void)
+#else
+#error Unknown architecture
+#endif
+{
+unsigned long phys = trampoline_phys;
+const int32_t *trampoline_ptr;
+
+/*
+ * Apply relocations to trampoline.
+ *
+ * This modifies the trampoline in place within Xen, so that it will
+ * operate correctly when copied into place.
+ */
+for ( trampoline_ptr = __trampoline_rel_start;
+  trampoline_ptr < __trampoline_rel_stop;
+  ++trampoline_ptr )
+*(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
+
+for ( trampoline_ptr = __trampoline_seg_start;
+  trampoline_ptr < __trampoline_seg_stop;
+  ++trampoline_ptr )
+*(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
+}
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 94f3443364..1acceec471 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/ef

[PATCH v4 1/6] x86/boot: Prep work for 32bit object changes

[Frediano Ziglio]

Broken out of the subsequent patch for clarity.

 * Rename head-bin-objs to obj32
 * Use a .32.o suffix to distinguish these objects
 * Factor out $(LD32)

No functional change.

Signed-off-by: Frediano Ziglio 
Signed-off-by: Andrew Cooper 
---
 xen/arch/x86/boot/Makefile | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index ff0f965876..1199291d2b 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,15 +1,16 @@
 obj-bin-y += head.o
 
-head-bin-objs := cmdline.o reloc.o
+obj32 := cmdline.32.o
+obj32 += reloc.32.o
 
-nocov-y   += $(head-bin-objs)
-noubsan-y += $(head-bin-objs)
-targets   += $(head-bin-objs)
+nocov-y   += $(obj32)
+noubsan-y += $(obj32)
+targets   += $(obj32)
 
-head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
+obj32 := $(addprefix $(obj)/,$(obj32))
 
 $(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(head-bin-objs:.o=.bin)
+$(obj)/head.o: $(obj32:.32.o=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -18,16 +19,20 @@ CFLAGS_x86_32 += -nostdinc -include $(filter 
%/include/xen/config.h,$(XEN_CFLAGS
 CFLAGS_x86_32 += $(filter -I% -O%,$(XEN_CFLAGS)) -D__XEN__
 
 # override for 32bit binaries
-$(head-bin-objs): CFLAGS_stack_boundary :=
-$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+$(obj32): CFLAGS_stack_boundary :=
+$(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+
+$(obj)/%.32.o: $(src)/%.c FORCE
+   $(call if_changed_rule,cc_o_c)
 
 LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
 LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
+LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
 
 %.bin: %.lnk
$(OBJCOPY) -j .text -O binary $< $@
 
-%.lnk: %.o $(src)/build32.lds
-   $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) 
-o $@ $<
+%.lnk: %.32.o $(src)/build32.lds
+   $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
 
 clean-files := *.lnk *.bin
-- 
2.34.1

[PATCH v4 6/6] x86/boot: Clarify comment

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 xen/arch/x86/boot/reloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index e50e161b27..e725cfb6eb 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -65,7 +65,7 @@ typedef struct memctx {
 /*
  * Simple bump allocator.
  *
- * It starts from the base of the trampoline and allocates downwards.
+ * It starts on top of space reserved for the trampoline and allocates 
downwards.
  */
 uint32_t ptr;
 } memctx;
-- 
2.34.1

Re: [RFC PATCH 01/13] PCI: Prepare removing devres from pci_intx()

[Philipp Stanner]

On Fri, 2024-10-11 at 16:50 +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 02:16:06PM +0200, Philipp Stanner wrote:
> > On Thu, 2024-10-10 at 17:40 +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 09, 2024 at 10:35:07AM +0200, Philipp Stanner wrote:
> > > > pci_intx() is a hybrid function which sometimes performs devres
> > > > operations, depending on whether pcim_enable_device() has been
> > > > used
> > > > to
> > > > enable the pci_dev. This sometimes-managed nature of the
> > > > function
> > > > is
> > > > problematic. Notably, it causes the function to allocate under
> > > > some
> > > > circumstances which makes it unusable from interrupt context.
> > > > 
> > > > To, ultimately, remove the hybrid nature from pci_intx(), it is
> > > > first
> > > > necessary to provide an always-managed and a never-managed
> > > > version
> > > > of that function. Then, all callers of pci_intx() can be ported
> > > > to
> > > > the
> > > > version they need, depending whether they use
> > > > pci_enable_device()
> > > > or
> > > > pcim_enable_device().
> 
> > > > An always-managed function exists, namely pcim_intx(), for
> > > > which
> > > > __pcim_intx(), a never-managed version of pci_intx() had been
> > > > implemented.
> > > 
> > > > Make __pcim_intx() a public function under the name
> > > > pci_intx_unmanaged(). Make pcim_intx() a public function.
> 
> It seems I got confused by these two paragraphs. Why the double
> underscored
> function is even mentioned here?

It's mentioned because it's being moved.

> 
> > > To avoid an additional churn we can make just completely new
> > > APIs,
> > > namely:
> > > pcim_int_x()
> > > pci_int_x()
> > > 
> > > You won't need all dirty dances with double underscored function
> > > naming and
> > > renaming.
> > 
> > Ähm.. I can't follow. The new version doesn't use double
> > underscores
> > anymore. __pcim_intx() is being removed, effectively.
> > After this series, we'd end up with a clean:
> > 
> > pci_intx() <-> pcim_intx()
> > 
> > just as in the other PCI APIs.
> 
> ...
> 
> > > > +   pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> > > > +
> > > > +   if (enable)
> > > > +   new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> > > > +   else
> > > > +   new = pci_command | PCI_COMMAND_INTX_DISABLE;
> > > > +
> > > > +   if (new != pci_command)
> > > 
> > > I would use positive conditionals as easy to read (yes, a couple
> > > of
> > > lines
> > > longer, but also a win is the indentation and avoiding an
> > > additional
> > > churn in
> > > the future in case we need to add something in this branch.
> > 
> > I can't follow. You mean:
> > 
> > if (new == pci_command)
> >     return;
> > 
> > ?
> > 
> > That's exactly the same level of indentation.
> 
> No, the body gets one level off.
> 
> > Plus, I just copied the code.
> > 
> > > > +   pci_write_config_word(pdev, PCI_COMMAND, new);
> 
>   if (new == pci_command)
>   return;
> 
>   pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> See the difference?
> Also, imaging adding a new code in your case:
> 
>   if (new != pci_command)
>   pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> ==>
> 
>   if (new != pci_command) {
>   ...foo...
>   pci_write_config_word(pdev, PCI_COMMAND, new);
>   ...bar...
>   }
> 
> And in mine:
> 
>   if (new == pci_command)
>   return;
> 
>   ...foo...
>   pci_write_config_word(pdev, PCI_COMMAND, new);
>   ...bar...
> 
> I hope it's clear now what I meant.

It is clear.. I'm not necessarily convinced that it's better to review
than just copying the pre-existing code, but if you really want it we
can do it I guess.

P.

Re: [PATCH] xen/public: increment domctl interface version

[Jan Beulich]

On 14.10.2024 09:36, Jürgen Groß wrote:
> On 14.10.24 09:14, Jan Beulich wrote:
>> On 14.10.2024 09:06, Juergen Gross wrote:
>>> The recent addition of the XEN_DOMCTL_dt_overlay function was missing
>>> the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
>>> first interface change of the 4.20 release cycle.
>>>
>>> Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device 
>>> attachment to domains")
>>
>> I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
>> bumped the version ...
>>
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -21,7 +21,7 @@
>>>   #include "hvm/save.h"
>>>   #include "memory.h"
>>>   
>>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
>>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x0018
>>
>> ... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
>> require a bump. Did you maybe mean to reference a different commit?
> 
> Oh, indeed. I wanted to reference d6e9a2aab39e.
> 
> And regarding to "plain additions don't require a bump": 4c733873b5c2 did
> a plain addition and bumped the version.

Right, hence why I said "for no apparent reason".

Jan

[PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()

[Andrew Cooper]

UBSAN complains:

  (XEN) 

  (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
  (XEN) load of address 82d040ae24c8 with insufficient space
  (XEN) for an object of type 'struct lock_profile *'
  (XEN) [ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:   C]

This shows up with GCC-14, but not with GCC-12.  I have not bisected further.

Either way, the types for __lock_profile_{start,end} are incorrect.

They are an array of struct lock_profile pointers.  Correct the extern's
types, and adjust the loop to match.

No practical change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Juergen Gross 
CC: Marek Marczykowski-Górecki 

Found as collatoral damage in
https://github.com/QubesOS/qubes-issues/issues/9501 but it's not related to
the main bug being repoted.

A reported-by tag TBC.
---
 xen/common/spinlock.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 0b877384451d..38caa10a2ea2 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -608,9 +608,6 @@ struct lock_profile_anc {
 typedef void lock_profile_subfunc(struct lock_profile *data, int32_t type,
 int32_t idx, void *par);
 
-extern struct lock_profile *__lock_profile_start;
-extern struct lock_profile *__lock_profile_end;
-
 static s_time_t lock_profile_start;
 static struct lock_profile_anc lock_profile_ancs[] = {
 [LOCKPROF_TYPE_GLOBAL] = { .name = "Global" },
@@ -780,13 +777,16 @@ void _lock_profile_deregister_struct(
 spin_unlock(&lock_profile_lock);
 }
 
+extern struct lock_profile *__lock_profile_start[];
+extern struct lock_profile *__lock_profile_end[];
+
 static int __init cf_check lock_prof_init(void)
 {
 struct lock_profile **q;
 
 BUILD_BUG_ON(ARRAY_SIZE(lock_profile_ancs) != LOCKPROF_TYPE_N);
 
-for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ )
+for ( q = __lock_profile_start; q < __lock_profile_end; q++ )
 {
 (*q)->next = lock_profile_glb_q.elem_q;
 lock_profile_glb_q.elem_q = *q;
-- 
2.39.5

Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Frediano Ziglio]

On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
 wrote:
>
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index 1199291d2b..23ad274c89 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-bin-y += head.o
> > +obj-bin-y += built_in_32.o
>
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.
>

It was cbundle.o before, but people preferred built_in_32.o.
It's a collection of object files like built_in.o so it does not seem
so bad to me.
But seen other replies, some other people prefer "bundle".

> > @@ -9,9 +10,6 @@ targets   += $(obj32)
> >
> >  obj32 := $(addprefix $(obj)/,$(obj32))
> >
> > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > -
> >  CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> >  $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> >  CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> >  $(obj)/%.32.o: $(src)/%.c FORCE
> >   $(call if_changed_rule,cc_o_c)
> >
> > +orphan-handling-$(call ld-option,--orphan-handling=error) := 
> > --orphan-handling=error
> >  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := 
> > --no-warn-rwx-segments
> >  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> >  LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> >
> > -%.bin: %.lnk
> > - $(OBJCOPY) -j .text -O binary $< $@
> > -
> > -%.lnk: %.32.o $(src)/build32.lds
> > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > -
> > -clean-files := *.lnk *.bin
> > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S 
> > FORCE
> > + $(call if_changed_dep,cpp_lds_S)
> > +
> > +# link all 32bit objects together
> > +$(obj)/built_in_32.tmp.o: $(obj32)
> > + $(LD32) -r -o $@ $^
> > +
> > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > +## link bundle with a given layout
>
> Could you put the comment aligned with the rest of the recipe? That way
> it won't visually separate the rule into several pieces. You can
> prefix it with @ so it doesn't show in build logs:
>
> @# comments
>

Yes, they look more or less the same but technically pretty different.
The "## XXX" is a comment for make command, the "\t@#comment" is a way
to tell make to not print the command before launching it so make will
launch a shell command "# comment".
Yes, indentation does not make it clear. Not that launching a shell to
execute a comment will take a long time. On the other hand, here that
comment is more for the rule than for the shell. Maybe something like

target:
  command \
   # do something

(personally I found it even more ugly)

> > + $(LD32) $(orphan-handling-y) -N -T $< -Map 
> > $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o 
> > $(obj)/built_in_32.tmp.o
>
> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst 
> %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
>
> :-(, maybe that's lots of $(patsubst,), not sure which is better between
> $(patsubst,) and using the stem $*.
>

Not strong about stem or patsubst.
The 2 filters seem good, they suggest lds for the script and objects
for the input, which makes sense.

> Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> remove it's prerequisite.
>
> BTW, everything is kind of temporary in a build system, beside the few
> files that we want to install on a machine, so having a target named
> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> remove them before the end of its recipe is fine (or those *tmp* aren't
> use outside of this recipe).
>

Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
So we still don't agree on one name, and now we want to find also
another, tricky.
More or less if it can help, one is a 32 bit object file that bundle
together multiple 32 bits object files while the other is the
conversion of the 32 bits bundle file to 64 bits.
XXX_32.o and XXX_32as64.o ??

> > +## extract binaries from object
> > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > + rm -f $(obj)/built_in_32.$(*F).o
> > +
> > +# generate final object file combining and checking above binaries
> > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin 
> > $(obj)/built_in_32.final.bin
>
> So, "other" isn't part of "final", I don't really know what those two
> things contains so naming wise I can't suggest anything useful.
>
> But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> it get regenerated if the script changes, or the .lds

[linux-linus test] 188081: tolerable FAIL - PUSHED

[osstest service owner]

flight 188081 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/188081/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 188073
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 188073
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 188073
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 188073
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 188073
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 188073
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass

version targeted for testing:
 linux6485cf5ea253d40d507cd71253c9568c5470cd27
baseline version:
 linux36c254515dc6592c44db77b84908358979dd6b50

Last test of basis   188073  2024-10-13 06:36:24 Z1 days
Failing since188077  2024-10-13 20:42:34 Z0 days2 attempts
Testing same since   188081  2024-10-14 05:19:18 Z0 days1 attempts


People who touched revisions under test:
  Alan Stern 
  Basavaraj Natikar 
  Benjamin Tissoires 
  Chris Hixon 
  Danilo Krummrich 
  Fiona Behrens 
  Greg Kroah-Hartman 
  Guilherme Giacomo Simoes 
  Icenowy Zheng 
  Jason Gerecke 
  Jinjie Ruan 
  Jiri Kosina 
  John Keeping 
  Jose Alberto Reguero 
  Kexy Biscuit 
  Linus Torvalds 
  Matthias Kaehlcke 
  Oliver Neukum 
  Pali Rohár 
  Radhey Shyam Pandey 
  Richard 
  Roy Luo 
  Selvarasu Ganesan 
  Skyler 
  Stefan Blum 
  Stefan Blum 
  Steve French 
  SurajSonawane2415 
  Thinh Nguyen 
  Wade Wang 
  WangYuli 
  Wentao Guan 

jobs:

[ovmf test] 188083: tolerable FAIL - PUSHED

[osstest service owner]

flight 188083 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/188083/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-qemuu-ovmf-amd64 16 guest-localmigrate fail REGR. vs. 
188065

version targeted for testing:
 ovmf 21767dcf4e04ade9e679f8562513da8ceedf19ec
baseline version:
 ovmf fcd9570c8d8164b42f907137a3a6e78977cc860a

Last test of basis   188065  2024-10-12 09:13:32 Z2 days
Testing same since   188083  2024-10-14 15:15:23 Z0 days1 attempts


People who touched revisions under test:
  Nhi Pham 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 fail



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   fcd9570c8d..21767dcf4e  21767dcf4e04ade9e679f8562513da8ceedf19ec -> 
xen-tested-master

Re: [PATCH] xen/public: increment domctl interface version

[Jan Beulich]

On 14.10.2024 09:06, Juergen Gross wrote:
> The recent addition of the XEN_DOMCTL_dt_overlay function was missing
> the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
> first interface change of the 4.20 release cycle.
> 
> Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device 
> attachment to domains")

I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
bumped the version ...

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x0018

... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
require a bump. Didi you maybe mean to reference a different commit?

Jan

Re: [PATCH] xen/public: increment domctl interface version

[Jan Beulich]

On 14.10.2024 09:14, Jan Beulich wrote:
> On 14.10.2024 09:06, Juergen Gross wrote:
>> The recent addition of the XEN_DOMCTL_dt_overlay function was missing
>> the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
>> first interface change of the 4.20 release cycle.
>>
>> Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device 
>> attachment to domains")
> 
> I'm confused: That change (a) pre-dates the branching of 4.20 and (b)
> bumped the version ...
> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -21,7 +21,7 @@
>>  #include "hvm/save.h"
>>  #include "memory.h"
>>  
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x0018
> 
> ... from 0x16 to 0x17. And for no apparent reason, as plain additions don't
> require a bump. Didi you maybe mean to reference a different commit?

Albeit looking at the history of domctl.h I also can't spot any candidate.

Jan

[PATCH] xen/public: increment domctl interface version

[Juergen Gross]

The recent addition of the XEN_DOMCTL_dt_overlay function was missing
the related update of XEN_DOMCTL_INTERFACE_VERSION, as it has been the
first interface change of the 4.20 release cycle.

Fixes: 4c733873b5c2 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment 
to domains")
Signed-off-by: Juergen Gross 
---
 xen/include/public/domctl.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e1028fc524..54cc9a06db 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0017
+#define XEN_DOMCTL_INTERFACE_VERSION 0x0018
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
-- 
2.43.0

[xen-unstable test] 188079: tolerable FAIL

[osstest service owner]

flight 188079 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/188079/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 188071
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 188071
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 188071
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 188071
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 188071
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 188071
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  76a54badf890f56ff72644593c0fbc72138e13aa
baseline version:
 xen  76a54badf890f56ff72644593c0fbc72138e13aa

Last test of basis   188079  2024-10-14 01:54:05 Z0 days
Testing same since  (not found) 0 attempts

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-l

Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

On 10/10/2024 15:03, Ayan Kumar Halder wrote:

Define enable_boot_cpu_mm() for the AArch64-V8R system.

Like boot-time page table in MMU system, we need a boot-time MPU protection
region configuration in MPU system so Xen can fetch code and data from normal
memory.

To do this, Xen maps the following sections of the binary as separate regions
(with permissions) :-
1. Text (Read only at EL2, execution is permitted)
2. RO data (Read only at EL2)
3. RO after init data and RW data (Read/Write at EL2)
4. Init Text (Read only at EL2, execution is permitted)
5. Init data and BSS (Read/Write at EL2)

Before creating a region, we check if the count exceeds the number defined in
MPUIR_EL2. If so, then the boot fails.

Also we check if the region is empty or not. IOW, if the start and end address
of a section is the same, we skip mapping the region.

To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
Registers", to get the definitions of these registers. Also, refer to G1.2
"Accessing MPU memory region registers", the following

```
The MPU provides two register interfaces to program the MPU regions:
- Access to any of the MPU regions via PRSELR_ELx, PRBAR_ELx, and
PRLAR_ELx.
```

We use the above mechanism for mapping sections to MPU memory regions.

MPU specific registers are defined in
xen/arch/arm/include/asm/arm64/mpu/sysregs.h.

Signed-off-by: Ayan Kumar Halder 
---
Changes from :-

v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
we have separate MPU regions for different parts of the Xen binary. The reason
being different regions will nned different permissions (as mentioned in the
linker script).

2. Introduced a label (__init_data_begin) to mark the beginning of the init data
section.

3. Moved MPU specific register definitions to mpu/sysregs.h.

4. Fixed coding style issues.

5. Included page.h in mpu/head.S as page.h includes sysregs.h.
I haven't seen sysregs.h included directly from head.S or mmu/head.S.
(Outstanding comment not addressed).

v2 - 1. Extracted "enable_mpu()" in a separate patch.

2. Removed alignment for limit address.

3. Merged some of the sections for preparing the early boot regions.

4. Checked for the max limit of MPU regions before creating a new region.

5. Checked for empty regions.

  xen/arch/arm/Makefile|   1 +
  xen/arch/arm/arm64/mpu/Makefile  |   1 +
  xen/arch/arm/arm64/mpu/head.S| 130 +++
  xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 
  xen/arch/arm/include/asm/mm.h|   2 +
  xen/arch/arm/include/asm/mpu/arm64/mm.h  |  22 
  xen/arch/arm/include/asm/mpu/mm.h|  20 +++
  xen/arch/arm/xen.lds.S   |   1 +
  8 files changed, 204 insertions(+)
  create mode 100644 xen/arch/arm/arm64/mpu/Makefile
  create mode 100644 xen/arch/arm/arm64/mpu/head.S
  create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
  create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
  create mode 100644 xen/arch/arm/include/asm/mpu/mm.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..aebccec63a 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -1,6 +1,7 @@
  obj-$(CONFIG_ARM_32) += arm32/
  obj-$(CONFIG_ARM_64) += arm64/
  obj-$(CONFIG_MMU) += mmu/
+obj-$(CONFIG_MPU) += mpu/


This change is incorrect. The correct change should have been in :-

--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -1,5 +1,6 @@
 obj-y += lib/
 obj-$(CONFIG_MMU) += mmu/
+obj-$(CONFIG_MPU) += mpu/

 obj-y += cache.o

I will wait for comments from reviewers before re-spinning the patch.

- Ayan

Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Jan Beulich]

On 14.10.2024 17:31, Anthony PERARD wrote:
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>> index 1199291d2b..23ad274c89 100644
>> --- a/xen/arch/x86/boot/Makefile
>> +++ b/xen/arch/x86/boot/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-bin-y += head.o
>> +obj-bin-y += built_in_32.o
> 
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.

blob32.o? bundle32.o?

Jan

Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Andrew Cooper]

On 14/10/2024 4:31 pm, Anthony PERARD wrote:
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>> index 1199291d2b..23ad274c89 100644
>> --- a/xen/arch/x86/boot/Makefile
>> +++ b/xen/arch/x86/boot/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-bin-y += head.o
>> +obj-bin-y += built_in_32.o
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.

Similarity was the point of suggesting it.

It is the end result of "the 32bit objects we've done magic with to
shoehorn into 64bit", and a improvement on "cbundle" from earlier
iterations.

~Andrew

Re: [PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()

[Jürgen Groß]

On 14.10.24 16:57, Andrew Cooper wrote:

UBSAN complains:

   (XEN) 

   (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
   (XEN) load of address 82d040ae24c8 with insufficient space
   (XEN) for an object of type 'struct lock_profile *'
   (XEN) [ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:   C]

This shows up with GCC-14, but not with GCC-12.  I have not bisected further.

Either way, the types for __lock_profile_{start,end} are incorrect.

They are an array of struct lock_profile pointers.  Correct the extern's
types, and adjust the loop to match.

No practical change.

Signed-off-by: Andrew Cooper 


Reviewed-by: Juergen Gross 


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()

[Andrew Cooper]

On 14/10/2024 4:12 pm, Jürgen Groß wrote:
> On 14.10.24 16:57, Andrew Cooper wrote:
>> UBSAN complains:
>>
>>    (XEN)
>> 
>>    (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
>>    (XEN) load of address 82d040ae24c8 with insufficient space
>>    (XEN) for an object of type 'struct lock_profile *'
>>    (XEN) [ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:  
>> C    ]
>>
>> This shows up with GCC-14, but not with GCC-12.  I have not bisected
>> further.
>>
>> Either way, the types for __lock_profile_{start,end} are incorrect.
>>
>> They are an array of struct lock_profile pointers.  Correct the extern's
>> types, and adjust the loop to match.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper 
>
> Reviewed-by: Juergen Gross 

Thanks, and I've got proper Reported-by tag from the conversation on Matrix.

~Andrew

Re: [PATCH v3 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Luca Fancellu]

+ Frediano for suggestion about header protection define name

> +++ b/xen/arch/arm/include/asm/mpu/layout.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MPU_LAYOUT_H__
> +#define __ARM_MPU_LAYOUT_H__

I think we are moving away from this notation:
https://patchwork.kernel.org/project/xen-devel/patch/20241004081713.749031-6-frediano.zig...@cloud.com/
Shall this be ASM___ARM__MPU__LAYOUT_H ? @Frediano


> +
> +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> +
> +/*
> + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This
> + * address indicates where Xen image will be loaded and run from. This
> + * address must be aligned to a PAGE_SIZE.
> + */
> +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
> +#error "XEN_START_ADDRESS must be aligned to 4KB"
> +#endif
> +
> +/*
> + * For MPU, XEN's virtual start address is same as the physical address.
> + * The reason being MPU treats VA == PA. IOW, it cannot map the physical
> + * address to a different fixed virtual address. So, the virtual start
> + * address is determined by the physical address at which Xen is loaded.
> + */
> +#define XEN_VIRT_START _AT(paddr_t, XEN_START_ADDRESS)
> +
> +#endif /* __ARM_MPU_LAYOUT_H__ */

   ^-- ASM___ARM__MPU__LAYOUT_H ? 

Apart from that, the rest looks ok to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v3 2/6] xen/arm: mpu: Introduce choice between MMU and MPU

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> There are features in the forthcoming patches which are dependent on
> MPU. For eg fixed start address.
> Also, some of the Xen features (eg STATIC_MEMORY) will be selected
> by the MPU configuration.
> 
> Thus, this patch introduces a choice between MMU and MPU for the type
> of memory management system. By default, MMU is selected.
> MPU is now gated by UNSUPPORTED.
> 
> Update SUPPORT.md to state that the support for MPU is experimental.

Maybe I’m wrong, but shouldn’t we add this only when MPU is supported?
In this case we are just having the Kconfig setting.

Apart from that, the rest looks good to me:

Reviewed-by: Luca Fancellu 

Re: [PATCH v3 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> If the BSS section is empty, then the function can just return.
> 
> Signed-off-by: Ayan Kumar Halder 

Reviewed-by: Luca Fancellu mailto:luca.fance...@arm.com>>

[xen-unstable-smoke test] 188084: tolerable all pass - PUSHED

[osstest service owner]

flight 188084 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/188084/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  542ac112fc68c66cfafc577e252404c21da4f75b
baseline version:
 xen  76a54badf890f56ff72644593c0fbc72138e13aa

Last test of basis   188054  2024-10-11 11:02:15 Z3 days
Testing same since   188084  2024-10-14 16:00:23 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   76a54badf8..542ac112fc  542ac112fc68c66cfafc577e252404c21da4f75b -> smoke

Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,


> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 00..4a21bc815c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include 
> +#include 
> +
> +#define REGION_TEXT_PRBAR   0x38/* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR 0x3A/* SH=11 AP=10 XN=10 */

NIT alignment

> +#define REGION_DATA_PRBAR   0x32/* SH=11 AP=00 XN=10 */
> +
> +#define REGION_NORMAL_PRLAR 0x0f/* NS=0 ATTR=111 EN=1 */
> +
> +/*
> + * Macro to prepare and set a EL2 MPU memory region.
> + * We will also create an according MPU memory region entry, which
> + * is a structure of pr_t,  in table \prmap.
> + *
> + * Inputs:
> + * sel: region selector
> + * base:reg storing base address (should be page-aligned)
> + * limit:   reg storing limit address
> + * prbar:   store computed PRBAR_EL2 value
> + * prlar:   store computed PRLAR_EL2 value
> + * maxcount:maximum number of EL2 regions supported
> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it 
> will be
> + *  REGION_DATA_PRBAR
> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it 
> will be
> + *  REGION_NORMAL_PRLAR
> + */
> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, 
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +
> +/* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +add   \sel, \sel, #1

I think there is an issue adding 1 here, because the very first region we are 
going to fill will be the 1st even if we intended the 0th.
Probably moving this one at the end will fix the issue

> +cmp   \sel, \maxcount
> +bgt   fail
> +
> +/* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> +and   \base, \base, #MPU_REGION_MASK
> +mov   \prbar, #\attr_prbar
> +orr   \prbar, \prbar, \base
> +
> +/* Limit address should be inclusive */
> +sub   \limit, \limit, #1
> +and   \limit, \limit, #MPU_REGION_MASK
> +mov   \prlar, #\attr_prlar
> +orr   \prlar, \prlar, \limit
> +
> +msr   PRSELR_EL2, \sel
> +isb
> +msr   PRBAR_EL2, \prbar
> +msr   PRLAR_EL2, \prlar
> +dsb   sy
> +isb
> +.endm
> +
> +/* Load the physical address of a symbol into xb */
> +.macro load_paddr xb, sym
> +ldr \xb, =\sym
> +add \xb, \xb, x20   /* x20 - Phys offset */
> +.endm
> +
> +/*
> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Inputs:
> + *   lr : Address to return to.
> + *
> + * Clobbers x0 - x5
> + *
> + */
> +FUNC(enable_boot_cpu_mm)
> +
> +/* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +mrs   x5, MPUIR_EL2
> +
> +/* x0: region sel */
> +mov   x0, xzr
> +/* Xen text section. */
> +load_paddr x1, _stext
> +load_paddr x2, _etext
> +cmp x1, x2
> +beq 1f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +1:  /* Xen read-only data section. */
> +load_paddr x1, _srodata
> +load_paddr x2, _erodata
> +cmp x1, x2
> +beq 2f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +2:  /* Xen read-only after init and data section. (RW data) */
> +load_paddr x1, __ro_after_init_start
> +load_paddr x2, __init_begin
> +cmp x1, x2
> +beq 3f
> +prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +3:  /* Xen code section. */
> +load_paddr x1, __init_begin
> +load_paddr x2, __init_data_begin
> +cmp x1, x2
> +beq 4f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +4:  /* Xen data and BSS section. */
> +load_paddr x1, __init_data_begin
> +load_paddr x2, __bss_end
> +cmp x1, x2
> +beq 5f
> +prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +5:
> +ret
> +
> +fail:
> +PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
> +wfe
> +b   1b
> +END(enable_boot_cpu_mm)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h 
> b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> new file mode 100644
> index 00..b0c31a58ec
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
> +#define __ASM_ARM_ARM64_MPU_SYSREGS_H

Same comment about define name as in patch 3, here and in every
new file of this patch

Cheers,
Luca

Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it

[Anthony PERARD]

On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 1199291d2b..23ad274c89 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
>  obj-bin-y += head.o
> +obj-bin-y += built_in_32.o

I don't quite like that this new object name is "built_in_32.o",
It's really closed to "built_in.*" which is used by Rules.mk to collect
all objects in a subdirectory. I don't really have a better suggestion at
the moment.

> @@ -9,9 +10,6 @@ targets   += $(obj32)
>  
>  obj32 := $(addprefix $(obj)/,$(obj32))
>  
> -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(obj32:.32.o=.bin)
> -
>  CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
>  $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
>  $(obj)/%.32.o: $(src)/%.c FORCE
>   $(call if_changed_rule,cc_o_c)
>  
> +orphan-handling-$(call ld-option,--orphan-handling=error) := 
> --orphan-handling=error
>  LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := 
> --no-warn-rwx-segments
>  LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
>  LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
>  
> -%.bin: %.lnk
> - $(OBJCOPY) -j .text -O binary $< $@
> -
> -%.lnk: %.32.o $(src)/build32.lds
> - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> -
> -clean-files := *.lnk *.bin
> +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> + $(call if_changed_dep,cpp_lds_S)
> +
> +# link all 32bit objects together
> +$(obj)/built_in_32.tmp.o: $(obj32)
> + $(LD32) -r -o $@ $^
> +
> +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> +## link bundle with a given layout

Could you put the comment aligned with the rest of the recipe? That way
it won't visually separate the rule into several pieces. You can
prefix it with @ so it doesn't show in build logs:

@# comments

> + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map 
> -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o

I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) 
-o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)

:-(, maybe that's lots of $(patsubst,), not sure which is better between
$(patsubst,) and using the stem $*.

Also, if something tries to use built_in_32.tmp.bin, we have a rule that
remove it's prerequisite.

BTW, everything is kind of temporary in a build system, beside the few
files that we want to install on a machine, so having a target named
with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
remove them before the end of its recipe is fine (or those *tmp* aren't
use outside of this recipe).

> +## extract binaries from object
> + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> + rm -f $(obj)/built_in_32.$(*F).o
> +
> +# generate final object file combining and checking above binaries
> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin 
> $(obj)/built_in_32.final.bin

So, "other" isn't part of "final", I don't really know what those two
things contains so naming wise I can't suggest anything useful.

But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
it get regenerated if the script changes, or the .lds that --script
option seems to use? What is the "--map" option, an input or output?
But I guess we can ignore the ".map" file because it's part of the
".bin".

Another thing that might be useful to do, is to use the "if_changed"
make macro, that way if the command line of the script changes, make
can remake the output. But maybe it's a bit complicated for this recipe
because it doesn't uses $< or $^.

> + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> + --script $(obj)/build32.final.lds \
> + --bin1 $(obj)/built_in_32.other.bin \
> + --bin2 $(obj)/built_in_32.final.bin \
> + --map $(obj)/built_in_32.final.map \
> + --exports cmdline_parse_early,reloc \
> + --output $@
> +
> +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> similarity index 70%
> rename from xen/arch/x86/boot/build32.lds
> rename to xen/arch/x86/boot/build32.lds.S
> index 56edaa727b..50c167aef0 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -15,22 +15,47 @@
>   * with this program.  If not, see .
>   */
>  
> -ENTRY(_start)
> +#ifdef FINAL
> +# define GAP 0
> +# define MULT 0
> +# define TEXT_START
> +#else
> +# define GAP 0x010200
> +# define MULT 1
> +# define TEXT_START 0x408020
> +#endif
> +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)