date:20221231


On 12/31/22 20:55, Mika Penttilä wrote:


If decompression does clobber the data, then we *also* need to figure 
out why that is. There are basically three possibilities:


1. If physical KASLR is NOT used:

 a. The boot loader doesn't honor the kernel safe area properly;
 b. Somewhere in the process a bug in the calculation of the
    kernel safe area has crept in.

2. If physical KASLR IS used:

 The decompressor doesn't correctly keep track of nor relocate
 all the keep-out zones before picking a target address.


Seems setup_data is not included in those mem_avoid regions.



[facepalm]




One is a bootloader bug, two is a kernel bugs. My guess is (2) is the 
culprit, but (1b) should be checked, too.




Correction: two are kernel bugs, i.e. (1b) and (2) are both kernel bugs.

-hpa

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data





On 12/31/22 10:22, Jason A. Donenfeld wrote:

On Sat, Dec 31, 2022 at 03:24:32PM +0100, Borislav Petkov wrote:

On Sat, Dec 31, 2022 at 02:51:28PM +0100, Jason A. Donenfeld wrote:

That failure is unrelated to the ident mapping issue Peter and
I discussed. The original failure is described in the commit message:
decompression clobbers the data, so sd->next points to garbage.


Right


So with that understanding confirmed, I'm confused at your surprise that
hpa's unrelated fix to the different issue didn't fix this issue.



If decompression does clobber the data, then we *also* need to figure 
out why that is. There are basically three possibilities:


1. If physical KASLR is NOT used:

a. The boot loader doesn't honor the kernel safe area properly;
b. Somewhere in the process a bug in the calculation of the
   kernel safe area has crept in.

2. If physical KASLR IS used:

The decompressor doesn't correctly keep track of nor relocate
all the keep-out zones before picking a target address.

One is a bootloader bug, two is a kernel bug. My guess is (2) is the 
culprit, but (1b) should be checked, too.


-hpa

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data


On 12/31/22 19:21, H. Peter Anvin wrote:



Alternatively, setup_data could be relocated, the boot param protocol
could be bumped, and then QEMU could conditionalized it's use of
setup_data based on that protocol version. That'd work, but seems a bit
more involved.


I think this is at least worth considering because the kernel overwriting
setup_data after having been told where that setup_data is located is 
not really

nice.

Well not explicitly at least - it gets the pointer to the first 
element and
something needs to traverse that list to know which addresses are 
live. But

still, that info is there so perhaps we need to take setup_data into
consideration too before decompressing...



It would probably be a good idea to add a "maximum physical address for 
initrd/setup_data/cmdline" field to struct kernel_info, though. It 
appears right now that those fields are being identity-mapped in the 
decompressor, and that means that if 48-bit addressing is used, physical 
memory may extend past the addressable range.


-hpa

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On 12/31/22 11:00, Borislav Petkov wrote:

On Sat, Dec 31, 2022 at 07:22:47PM +0100, Jason A. Donenfeld wrote:

So with that understanding confirmed, I'm confused at your surprise that
hpa's unrelated fix to the different issue didn't fix this issue.

No surprise there - I used a qemu variant without your patch to prevent the
setup_data clobbering so hpa's fix can't help there.

But since the kernel doesn't do this now, and the 62MiB bug also seems
to apply to existing kernels, for the purposes of QEMU for now, I think
the v3 patch is probably best, since it'll handle existing kernels.

Right, we can't fix all those guests which are out there.

Alternatively, setup_data could be relocated, the boot param protocol
could be bumped, and then QEMU could conditionalized it's use of
setup_data based on that protocol version. That'd work, but seems a bit
more involved.

I think this is at least worth considering because the kernel overwriting
setup_data after having been told where that setup_data is located is not really
nice.

Well not explicitly at least - it gets the pointer to the first element and
something needs to traverse that list to know which addresses are live. But
still, that info is there so perhaps we need to take setup_data into
consideration too before decompressing...

As far as the decompression itself goes, it should only a problem if we
are using physical KASLR since otherwise the kernel has a guaranteed
safe zone already allocated by the boot loader. However, if physical
KASLR is in use, then the decompressor needs to know everything there is
to know about the memory map.

However, there also seems to be some kind of interaction with AMD SEV-SNP.

The bug appears to have been introduced by:

b57feed2cc2622ae14b2fa62f19e973e5e0a60cf
x86/compressed/64: Add identity mappings for setup_data entries
https://lore.kernel.org/r/tycpr01mb694815cd815e98945f63c99183...@tycpr01mb6948.jpnprd01.prod.outlook.com

... which was included in version 5.19, so it is relatively recent.

For a small amount of setup_data, the solution of just putting it next
to the command line makes a lot of sense, and should be safe indefinitely.

-hpa

RE: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

2022-12-31 Thread Dong, Eddie

> When booting the Zephyr demo in [1] we get:
> 
>   aspeed.io: unimplemented device write (size 4, offset 0x185128, value
> 0x030f1ff1) <--
>   aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
> 0x03f1)
> 
> This corresponds to this Zephyr code [2]:
> 
>   static int aspeed_wdt_init(const struct device *dev)
>   {
> const struct aspeed_wdt_config *config = dev->config;
> struct aspeed_wdt_data *const data = dev->data;
> uint32_t reg_val;
> 
> /* disable WDT by default */
> reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
> reg_val &= ~WDT_CTRL_ENABLE;
> sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> 
> sys_write32(data->rst_mask1,
> config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <--
> sys_write32(data->rst_mask2,
> config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> 
> return 0;
>   }
> 
> The register definitions are [3]:
> 
>   #define WDT_RELOAD_VAL_REG  0x0004
>   #define WDT_RESTART_REG 0x0008
>   #define WDT_CTRL_REG0x000C
>   #define WDT_TIMEOUT_STATUS_REG  0x0010
>   #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>   #define WDT_RESET_MASK1_REG 0x001C
>   #define WDT_RESET_MASK2_REG 0x0020
>   #define WDT_SW_RESET_MASK1_REG  0x0028   <--
>   #define WDT_SW_RESET_MASK2_REG  0x002C
>   #define WDT_SW_RESET_CTRL_REG   0x0024
> 
> Currently QEMU only cover a MMIO region of size 0x20:
> 
>   #define ASPEED_WDT_REGS_MAX(0x20 / 4)
> 
> Change to map the whole 'iosize' which might be bigger, covering the other

The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
Probably the Qemu is emulating an old version of the hardware.

Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
while iosize is for all devices, and its initial value comes from the per 
device type REGS_MAX.

> registers. The MemoryRegionOps read/write handlers will report the accesses
> as out-of-bounds guest-errors, but the next commit will report them as
> unimplemented.
> 
> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> [3] https://github.com/AspeedTech-
> BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> 
> Reviewed-by: Peter Delevoryas 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/watchdog/wdt_aspeed.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> 958725a1b5..eefca31ae4 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> Error **errp)  {
>  SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  AspeedWDTState *s = ASPEED_WDT(dev);
> +AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
> 
>  assert(s->scu);
> 
> @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> Error **errp)
>  s->pclk_freq = PCLK_HZ;
> 
>  memory_region_init_io(>iomem, OBJECT(s), _wdt_ops, s,
> -  TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> +  TYPE_ASPEED_WDT, awc->iosize);
>  sysbus_init_mmio(sbd, >iomem);
>  }
> 
> --
> 2.38.1
>

RE: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

2022-12-31 Thread Dong, Eddie

> 
> Avoid confusing two different things:
> - the WDT I/O region size ('iosize')
> - at which offset the SoC map the WDT ('offset') While it is often the same, 
> we
> can map smaller region sizes at larger offsets.
> 
> Here we are interested in the I/O region size, so rename as 'iosize'.
> 
> Reviewed-by: Peter Delevoryas 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/aspeed_ast10x0.c  | 2 +-
>  hw/arm/aspeed_ast2600.c  | 2 +-
>  hw/arm/aspeed_soc.c  | 2 +-
>  hw/watchdog/wdt_aspeed.c | 8 
>  include/hw/watchdog/wdt_aspeed.h | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
> 4d0b9b115f..122b3fd3f3 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState
> *dev_soc, Error **errp)
>  return;
>  }
>  aspeed_mmio_map(s, SYS_BUS_DEVICE(>wdt[i]), 0,
> -sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);


How does the specification say here (I didn't find it)?

I read this is for a case where multiple WDTs are implemented. 
And offset means the distance io registers of WDT[1] are located from WDT[0].
Or does the spec explicitly says the io registers of WDT[1] is located right 
after
WDT[0] without any gaps in this case, iosize is better)?

>  }
> 
>  /* GPIO */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
> cd75465c2b..a79e05ddbd 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
>  return;
>  }
>  aspeed_mmio_map(s, SYS_BUS_DEVICE(>wdt[i]), 0,
> -sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>  }
> 
>  /* RAM */
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c index
> b05b9dd416..2c0924d311 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev,
> Error **errp)
>  return;
>  }
>  aspeed_mmio_map(s, SYS_BUS_DEVICE(>wdt[i]), 0,
> -sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>  }
> 
>  /* RAM  */
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> d753693a2e..958725a1b5 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass
> *klass, void *data)
>  AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>  dc->desc = "ASPEED 2400 Watchdog Controller";
> -awc->offset = 0x20;
> +awc->iosize = 0x20;
>  awc->ext_pulse_width_mask = 0xff;
>  awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>  awc->wdt_reload = aspeed_wdt_reload; @@ -346,7 +346,7 @@ static void
> aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>  AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>  dc->desc = "ASPEED 2500 Watchdog Controller";
> -awc->offset = 0x20;
> +awc->iosize = 0x20;
>  awc->ext_pulse_width_mask = 0xf;
>  awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>  awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -369,7 +369,7
> @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>  AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>  dc->desc = "ASPEED 2600 Watchdog Controller";
> -awc->offset = 0x40;
> +awc->iosize = 0x40;
>  awc->ext_pulse_width_mask = 0xf; /* TODO */
>  awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>  awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -392,7 +392,7
> @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
>  AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>  dc->desc = "ASPEED 1030 Watchdog Controller";
> -awc->offset = 0x80;
> +awc->iosize = 0x80;
>  awc->ext_pulse_width_mask = 0xf; /* TODO */
>  awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>  awc->reset_pulse = aspeed_2500_wdt_reset_pulse; diff --git
> a/include/hw/watchdog/wdt_aspeed.h
> b/include/hw/watchdog/wdt_aspeed.h
> index dfa5dfa424..db91ee6b51 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -40,7 +40,7 @@ struct AspeedWDTState {  struct AspeedWDTClass {
>  SysBusDeviceClass parent_class;
> 
> -uint32_t offset;
> +uint32_t iosize;
>  uint32_t ext_pulse_width_mask;
>  uint32_t reset_ctrl_reg;
>  void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
> --
> 2.38.1
>

Re: Question about TLBs

2022-12-31 Thread Alex Bennée

Nada Lachtar  writes:

> [[S/MIME Signed Part:Undecided]]
> Hello,
>
> Does Qemu maintain two TLB for the x86_64 system (i.e iTLB and dTLB)?
> If yes, can you please point me to how to access the dTLB and what
> data structure maintains this information!

Qemu's internal softmmu TLB is unified and holds information for code
loads, memory accesses and re-directions for io access. See CPUTLBEntry
and CPUTLBEntryFull. They do not directly map onto any target specific
structures (although targets can use TARGET_PAGE_ENTRY_EXTRA to store
extra information in the table).

These entries are filled by target specific code via
cpu->cc->tcg_ops->tlb_fill() which in the x86 case takes you to
x86_cpu_tlb_fill(). This is the code responsible to walking the target
page tables and filling in the details of the mappings.

>
> I would appreciate any help,
>
>
> Thanks,
>
>
>
>
> [[End of S/MIME Signed Part]]

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 07:22:47PM +0100, Jason A. Donenfeld wrote:
> So with that understanding confirmed, I'm confused at your surprise that
> hpa's unrelated fix to the different issue didn't fix this issue.

No surprise there - I used a qemu variant without your patch to prevent the
setup_data clobbering so hpa's fix can't help there.

> But since the kernel doesn't do this now, and the 62MiB bug also seems
> to apply to existing kernels, for the purposes of QEMU for now, I think
> the v3 patch is probably best, since it'll handle existing kernels.

Right, we can't fix all those guests which are out there.

> Alternatively, setup_data could be relocated, the boot param protocol
> could be bumped, and then QEMU could conditionalized it's use of
> setup_data based on that protocol version. That'd work, but seems a bit
> more involved.

I think this is at least worth considering because the kernel overwriting
setup_data after having been told where that setup_data is located is not really
nice.

Well not explicitly at least - it gets the pointer to the first element and
something needs to traverse that list to know which addresses are live. But
still, that info is there so perhaps we need to take setup_data into
consideration too before decompressing...

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 03:24:32PM +0100, Borislav Petkov wrote:
> On Sat, Dec 31, 2022 at 02:51:28PM +0100, Jason A. Donenfeld wrote:
> > That failure is unrelated to the ident mapping issue Peter and
> > I discussed. The original failure is described in the commit message:
> > decompression clobbers the data, so sd->next points to garbage.
> 
> Right

So with that understanding confirmed, I'm confused at your surprise that
hpa's unrelated fix to the different issue didn't fix this issue.

> and the fact that the kernel overwrites it still feels kinda wrong: the
> kernel knows where setup_data is - the address is in the setup header so
> *actually*, it should take care of not to clobber it.

Yea, technically the bootloader could relocate all the setup_data links
by copying them and updating ->next. This wouldn't be so hard to do.
(Special care would have to be taken, though, to zero out
SETUP_RNG_SEED, though, for forward secrecy and such.)

But since the kernel doesn't do this now, and the 62MiB bug also seems
to apply to existing kernels, for the purposes of QEMU for now, I think
the v3 patch is probably best, since it'll handle existing kernels.
Alternatively, setup_data could be relocated, the boot param protocol
could be bumped, and then QEMU could conditionalized it's use of
setup_data based on that protocol version. That'd work, but seems a bit
more involved.

So maybe for now, v3 works? Hopefully that looks like a correct approach
to hpa, anyhow:
https://lore.kernel.org/lkml/20221230220725.618763-1-ja...@zx2c4.com/
I think it should fit with what he described would work.

Jason

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 02:51:28PM +0100, Jason A. Donenfeld wrote:
> That failure is unrelated to the ident mapping issue Peter and
> I discussed. The original failure is described in the commit message:
> decompression clobbers the data, so sd->next points to garbage.

Right, and the fact that the kernel overwrites it still feels kinda wrong: the
kernel knows where setup_data is - the address is in the setup header so
*actually*, it should take care of not to clobber it.

But hpa would correct me if I'm missing an aspect about whose responsibility it
is to do what here...

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PULL 17/21] target/hexagon: prepare input for the idef-parser

2022-12-31 Thread Alessandro Di Federico via

On Thu, 29 Dec 2022 11:41:22 +0100
Thomas Huth  wrote:

> I'm now seeing changes to scripts/meson-buildoptions.sh after
> rebuilding QEMU ... looks like you likely forgot to update that file
> with the automatic update after changing meson_options.txt ?

Ah, there seems to be another couple of unrelated missing updates to
scripts/meson-buildoptions.sh.

`Makefile` seems to rely on modification dates of files in the source
tree, but git does not track those.

git clone ...
mkdir build
./configure ...
make update-buildoptions # Does nothing
touch ../meson_options.txt
make update-buildoptions # Actually makes changes

Also, AFAIU the `update-buildoptions` target is not available in ninja.
However maybe this is not a bug since the doc says to use make.

Anyway, patch coming soon!

-- 
Alessandro Di Federico
rev.ng Labs

Re: [PATCH 1/1] target/hexagon: work around unused variable in yyparser

2022-12-31 Thread Alessandro Di Federico via

On Sat, 31 Dec 2022 17:19:35 +0800
Zongyuan Li  wrote:

> Variable 'yynerrs' is recognized as unused variable in clang15,

This issue is already handled by another similar patch (target/hexagon:
suppress unused variable warning) that will soon end up in a pull
request.

Thanks for looking into this though.

-- 
Alessandro Di Federico
rev.ng Labs

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 02:48:12PM +0100, Borislav Petkov wrote:
> On Sat, Dec 31, 2022 at 02:44:08PM +0100, Jason A. Donenfeld wrote:
> > Are you using patch v1 minus the 62 MiB thing?
> 
> No, I want to see the original failure - the one that prompted you to send
> 
> https://lore.kernel.org/r/20221228143831.396245-1-ja...@zx2c4.com

That failure is unrelated to the ident mapping issue Peter and
I discussed. The original failure is described in the commit message:
decompression clobbers the data, so sd->next points to garbage.

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 02:44:08PM +0100, Jason A. Donenfeld wrote:
> Are you using patch v1 minus the 62 MiB thing?

No, I want to see the original failure - the one that prompted you to send

https://lore.kernel.org/r/20221228143831.396245-1-ja...@zx2c4.com

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/3] hw/mips/malta: Set PIIX4 IRQ routes in embedded bootloader

2022-12-31 Thread Philippe Mathieu-Daudé


On 31/12/22 10:53, Bernhard Beschow wrote:



Am 21. November 2022 15:34:05 UTC schrieb Bernhard Beschow :



Am 27. Oktober 2022 20:47:19 UTC schrieb "Philippe Mathieu-Daudé" 
:

Linux kernel expects the northbridge & southbridge chipsets
configured by the BIOS firmware. We emulate that by writing
a tiny bootloader code in write_bootloader().

Upon introduction in commit 5c2b87e34d ("PIIX4 support"),
the PIIX4 configuration space included values specific to
the Malta board.

Set the Malta-specific IRQ routing values in the embedded
bootloader, so the next commit can remove the Malta specific
bits from the PIIX4 PCI-ISA bridge and make it generic
(matching the real hardware).

Signed-off-by: Philippe Mathieu-Daudé 
---
FIXME: Missing the nanoMIPS counter-part!


Who will be taking care of this? I have absolutely no clue how the 
write_bootloader functions work, so I don't see how to fix it.


Ping


This comment has been taken care of:
https://lore.kernel.org/qemu-devel/a3c3f639-dbb1-88a7-43fe-547a234c5...@linaro.org/

However while testing the MIPS pull request I prepared I
found a bug in the GT64120 which I'm trying to fix since
various days... Unfortunately your series depends on it,
so this is a blocking issue. Sorry for this long delay...

Phil.

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 02:40:59PM +0100, Borislav Petkov wrote:
> On Fri, Dec 30, 2022 at 05:06:55PM -0800, H. Peter Anvin wrote:
> > This needs to be something like:
> > 
> > kernel_add_identity_map(sd_addr, sd_addr + sizeof(*sd));
> > kernel_add_identity_map(sd_addr + sizeof(*sd),
> > sd_addr + sizeof(*sd) + sd->len);
> 
> It still #PFs with that:
> 
> (gdb) bt
> #0  0x84738576 in native_halt () at 
> ./arch/x86/include/asm/irqflags.h:57
> #1  halt () at ./arch/x86/include/asm/irqflags.h:98
> #2  early_fixup_exception (regs=regs@entry=0x84007dc8, 
> trapnr=trapnr@entry=14) at arch/x86/mm/extable.c:340
> #3  0x846ff465 in do_early_exception (regs=0x84007dc8, 
> trapnr=14) at arch/x86/kernel/head64.c:424
> #4  0x846ff14f in early_idt_handler_common () at 
> arch/x86/kernel/head_64.S:483
> #5  0xc149f9894908788d in ?? ()
> #6  0xff2003fc in ?? ()
> #7  0x0010 in fixed_percpu_data ()
> #8  0xdc00 in ?? ()
> #9  0x84007ea8 in init_thread_union ()
> #10 0xff20088d in ?? ()
> #11 0x in ?? ()
> 
> /me goes to dig more.

Are you using patch v1 minus the 62 MiB thing? If you haven't applied
patch v1 and then removed the 62 MiB limitation in it, then you've
misunderstood the conversation again.

Please see my reproduction steps to Peter:
https://lore.kernel.org/lkml/y68k4mpuz6edq...@zx2c4.com/

Jason

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 02:35:45PM +0100, Borislav Petkov wrote:
> On Sat, Dec 31, 2022 at 01:54:50PM +0100, Jason A. Donenfeld wrote:
> > Nothing special... `-kernel bzImage` should be enough to do it. Eric
> > reported it, and then I was able to repro trivially. Sure you got the
> > right version?
> 
> Yeah, qemu executables confusion here - had wrongly something older of the
> version 7.1...
> 
> Now made sure I'm actually booting with the latest qemu:
> 
> QEMU emulator version 7.2.50 (v7.2.0-333-g222059a0fccf)
> 
> With that the kernel with your config hangs early during boot and the stack
> trace is below.
> 
> Seeing how it says trapnr 14, then that looks like something you are seeing.
> 
> But lemme poke at it more.

Yes. The cause is what I've described in the commit message. There are
two proposed fixes, the v1, which has the 62 MiB limitation due to a
kernel bug, and the v3, which seems to work fine, and is simpler, and I
suspect that's the one QEMU upstream should take.

https://lore.kernel.org/lkml/20221230220725.618763-1-ja...@zx2c4.com/

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Fri, Dec 30, 2022 at 05:06:55PM -0800, H. Peter Anvin wrote:
> This needs to be something like:
> 
> kernel_add_identity_map(sd_addr, sd_addr + sizeof(*sd));
> kernel_add_identity_map(sd_addr + sizeof(*sd),
>   sd_addr + sizeof(*sd) + sd->len);

It still #PFs with that:

(gdb) bt
#0  0x84738576 in native_halt () at ./arch/x86/include/asm/irqflags.h:57
#1  halt () at ./arch/x86/include/asm/irqflags.h:98
#2  early_fixup_exception (regs=regs@entry=0x84007dc8, 
trapnr=trapnr@entry=14) at arch/x86/mm/extable.c:340
#3  0x846ff465 in do_early_exception (regs=0x84007dc8, 
trapnr=14) at arch/x86/kernel/head64.c:424
#4  0x846ff14f in early_idt_handler_common () at 
arch/x86/kernel/head_64.S:483
#5  0xc149f9894908788d in ?? ()
#6  0xff2003fc in ?? ()
#7  0x0010 in fixed_percpu_data ()
#8  0xdc00 in ?? ()
#9  0x84007ea8 in init_thread_union ()
#10 0xff20088d in ?? ()
#11 0x in ?? ()

/me goes to dig more.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 01:54:50PM +0100, Jason A. Donenfeld wrote:
> Nothing special... `-kernel bzImage` should be enough to do it. Eric
> reported it, and then I was able to repro trivially. Sure you got the
> right version?

Yeah, qemu executables confusion here - had wrongly something older of the
version 7.1...

Now made sure I'm actually booting with the latest qemu:

QEMU emulator version 7.2.50 (v7.2.0-333-g222059a0fccf)

With that the kernel with your config hangs early during boot and the stack
trace is below.

Seeing how it says trapnr 14, then that looks like something you are seeing.

But lemme poke at it more.

#0  0x84738576 in native_halt () at ./arch/x86/include/asm/irqflags.h:57
#1  halt () at ./arch/x86/include/asm/irqflags.h:98
#2  early_fixup_exception (regs=regs@entry=0x84007dc8, 
trapnr=trapnr@entry=14) at arch/x86/mm/extable.c:340
#3  0x846ff465 in do_early_exception (regs=0x84007dc8, 
trapnr=14) at arch/x86/kernel/head64.c:424
#4  0x846ff14f in early_idt_handler_common () at 
arch/x86/kernel/head_64.S:483
#5  0x2404c7410cd0 in ?? ()
#6  0xff20073c in ?? ()
#7  0x0010 in fixed_percpu_data ()
#8  0xdc00 in ?? ()
#9  0x84007ea8 in init_thread_union ()
#10 0xff200cd0 in ?? ()
#11 0x in ?? ()

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Fri, Dec 30, 2022 at 05:06:55PM -0800, H. Peter Anvin wrote:
> 
> 
> On 12/30/22 14:10, Jason A. Donenfeld wrote:
> > On Fri, Dec 30, 2022 at 01:58:39PM -0800, H. Peter Anvin wrote:
> >> See the other thread fork. They have identified the problem already.
> > 
> > Not sure I follow. Is there another thread where somebody worked out why
> > this 62meg limit was happening?
> > 
> > Note that I sent v2/v3, to fix the original problem in a different way,
> > and if that looks good to the QEMU maintainers, then we can all be happy
> > with that. But I *haven't* addressed and still don't fully understand
> > why the 62meg limit applied to my v1 in the way it does. Did you find a
> > bug there to fix? If so, please do CC me.
> > 
> 
> Yes, you yourself posted the problem:
> 
> > Then build qemu. Run it with `-kernel bzImage`, based on the kernel
> > built with the .config I attached.
> > 
> > You'll see that the CPU triple faults when hitting this line:
> > 
> > sd = (struct setup_data *)boot_params->hdr.setup_data;
> > while (sd) {
> > unsigned long sd_addr = (unsigned long)sd;
> > 
> > kernel_add_identity_map(sd_addr, sd_addr + sizeof(*sd) + 
> > sd->len);  <
> > sd = (struct setup_data *)sd->next;
> > }
> > 
> > , because it dereferences *sd. This does not happen if the decompressed
> > size of the kernel is < 62 megs.
> > 
> > So that's the "big and pretty serious" bug that might be worthy of
> > investigation.
> 
> This needs to be something like:
> 
> kernel_add_identity_map(sd_addr, sd_addr + sizeof(*sd));
> kernel_add_identity_map(sd_addr + sizeof(*sd),
>   sd_addr + sizeof(*sd) + sd->len);
> 

Oh, right, duh. Thanks for spelling it out.

Jason

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data

On Sat, Dec 31, 2022 at 10:48:16AM +0100, Borislav Petkov wrote:
> On Fri, Dec 30, 2022 at 04:59:30PM +0100, Jason A. Donenfeld wrote:
> > I'll attach a .config file. Apply the patch at the top of this thread to
> > qemu,
> 
> Hmm, so the patch at the top of this thread is fixing the clobbering of
> setup_data.
> 
> But I tried latest qemu with your .config and it boots ok here. So how do I
> repro the original issue you're trying to fix?

Nothing special... `-kernel bzImage` should be enough to do it. Eric
reported it, and then I was able to repro trivially. Sure you got the
right version?

Jason

Re: qemu-system-i386: Could not install MSR_CORE_THREAD_COUNT handler: Success

2022-12-31 Thread Vitaly Chikunov

Alexander,

On Sat, Dec 31, 2022 at 12:34:45PM +0100, Alexander Graf wrote:
> On 31.12.22 11:17, Vitaly Chikunov wrote:
> > On Sat, Dec 31, 2022 at 10:28:21AM +0100, Alexander Graf wrote:
> > > On 30.12.22 19:16, Vitaly Chikunov wrote:
> > > > On Fri, Dec 30, 2022 at 06:44:14PM +0100, Alexander Graf wrote:
> > > > > This is a kvm kernel bug and should be fixed with the latest stable 
> > > > > releases. Which kernel version are you running?
> > > > This is on latest v6.0 stable - 6.0.15.
> > > > 
> > > > Maybe there could be workaround for such situations? (Or maybe it's
> > > > possible to make this error non-fatal?) We use qemu+kvm for testing and
> > > > now we cannot test on x86.
> > > I'm confused what's going wrong for you. I tried to reproduce the issue
> > > locally, but am unable to:
> > > 
> > > $ uname -a
> > > Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
> > > 2022 x86_64 x86_64 x86_64 GNU/Linux
> > > $ linux32 chroot .
> > > $ uname -a
> > > Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
> > > 2022 i686 GNU/Linux
> > > $ cd qemu
> > > $ file ./build/qemu-system-i386
> > > ./build/qemu-system-i386: ELF 32-bit LSB shared object, Intel 80386, 
> > > version
> > > 1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.2, for 
> > > GNU/Linux
> > > 3.2.0, BuildID[sha1]=f75e20572be5c604c121de4497397665c168aa4c, with
> > > debug_info, not stripped
> > > $ ./build/qemu-system-i386 --version
> > > QEMU emulator version 7.2.0 (v7.2.0-dirty)
> > > Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
> > > $ ./build/qemu-system-i386 -nographic -enable-kvm
> > > SeaBIOS (version rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org)
> > > [...]
> > > 
> > > 
> > > Can you please double check whether your host kernel version is 6.0.15?
> > > Please paste the output of "uname -a".
> > Excuse me, I'm incorrectly reported kernel version I tried to boot instead
> > of host one. Host kernels are quite old, 5.15.59 and even 5.17.15 --
> > where failure is occurring.
> > 
> > I just tested on 5.15.85 and there is no failure.
> 
> 
> Awesome, great to hear :). That means everything works as expected at least.
> 
> 
> >builder@i586:/.in$ uname -a
> >Linux localhost.localdomain 5.15.85-std-def-alt1 #1 SMP Wed Dec 21 
> > 21:14:40 UTC 2022 i686 GNU/Linux
> >builder@i586:/.in$ qemu-system-i386 -nographic -enable-kvm
> >SeaBIOS (version 1.16.1-alt1)
> > 
> > Perhaps, one of solutions it to reboot our build fleet to newer kernels.
> > [This maybe hard, though, since special builder node image should be
> > created and reboot shall be coordinated through all systems, in compare,
> > updating QEMU would be easier since chroot is created on every build].
> 
> 
> I understand that it may be slightly painful to update your build fleet, but
> given this is a genuine kernel bug that has a fix available upstream and it
> only happens on niche corner cases (i386 QEMU on x86-64 Linux kernels with
> the bug) that I doubt anyone will use in production, I'd prefer we keep the
> QEMU logic as is :).
> 
> In the meanwhile, while you're patching the build fleet, you can apply the
> patch below as part of your build process to ensure you don't fail due to
> the kernel bug. Just make sure to remove it again as soon as you're done
> with the fleet update :).

Thanks for the suggestions.
Best wishes for the New Year!

Vitaly,

> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index a213209379..b9396bc7a6 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2632,7 +2632,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>  return ret;
>  }
>  }
> +#ifdef __x86_64__
>  if (kvm_vm_check_extension(s, KVM_CAP_X86_USER_SPACE_MSR)) {
> +#else
> +    if (0) {
> +#endif
>  bool r;
> 
>  ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,
> 
> Alex
>

Re: qemu-system-i386: Could not install MSR_CORE_THREAD_COUNT handler: Success

2022-12-31 Thread Alexander Graf


Hi Vitaly,

On 31.12.22 11:17, Vitaly Chikunov wrote:

Alexander,

On Sat, Dec 31, 2022 at 10:28:21AM +0100, Alexander Graf wrote:

On 30.12.22 19:16, Vitaly Chikunov wrote:

On Fri, Dec 30, 2022 at 06:44:14PM +0100, Alexander Graf wrote:

This is a kvm kernel bug and should be fixed with the latest stable releases. 
Which kernel version are you running?

This is on latest v6.0 stable - 6.0.15.

Maybe there could be workaround for such situations? (Or maybe it's
possible to make this error non-fatal?) We use qemu+kvm for testing and
now we cannot test on x86.

I'm confused what's going wrong for you. I tried to reproduce the issue
locally, but am unable to:

$ uname -a
Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
2022 x86_64 x86_64 x86_64 GNU/Linux
$ linux32 chroot .
$ uname -a
Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
2022 i686 GNU/Linux
$ cd qemu
$ file ./build/qemu-system-i386
./build/qemu-system-i386: ELF 32-bit LSB shared object, Intel 80386, version
1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.2, for GNU/Linux
3.2.0, BuildID[sha1]=f75e20572be5c604c121de4497397665c168aa4c, with
debug_info, not stripped
$ ./build/qemu-system-i386 --version
QEMU emulator version 7.2.0 (v7.2.0-dirty)
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
$ ./build/qemu-system-i386 -nographic -enable-kvm
SeaBIOS (version rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org)
[...]


Can you please double check whether your host kernel version is 6.0.15?
Please paste the output of "uname -a".

Excuse me, I'm incorrectly reported kernel version I tried to boot instead
of host one. Host kernels are quite old, 5.15.59 and even 5.17.15 --
where failure is occurring.

I just tested on 5.15.85 and there is no failure.



Awesome, great to hear :). That means everything works as expected at least.



   builder@i586:/.in$ uname -a
   Linux localhost.localdomain 5.15.85-std-def-alt1 #1 SMP Wed Dec 21 21:14:40 
UTC 2022 i686 GNU/Linux
   builder@i586:/.in$ qemu-system-i386 -nographic -enable-kvm
   SeaBIOS (version 1.16.1-alt1)

Perhaps, one of solutions it to reboot our build fleet to newer kernels.
[This maybe hard, though, since special builder node image should be
created and reboot shall be coordinated through all systems, in compare,
updating QEMU would be easier since chroot is created on every build].



I understand that it may be slightly painful to update your build fleet, 
but given this is a genuine kernel bug that has a fix available upstream 
and it only happens on niche corner cases (i386 QEMU on x86-64 Linux 
kernels with the bug) that I doubt anyone will use in production, I'd 
prefer we keep the QEMU logic as is :).


In the meanwhile, while you're patching the build fleet, you can apply 
the patch below as part of your build process to ensure you don't fail 
due to the kernel bug. Just make sure to remove it again as soon as 
you're done with the fleet update :).



diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index a213209379..b9396bc7a6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2632,7 +2632,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 return ret;
 }
 }
+#ifdef __x86_64__
 if (kvm_vm_check_extension(s, KVM_CAP_X86_USER_SPACE_MSR)) {
+#else
+    if (0) {
+#endif
 bool r;

 ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,

Alex

Re: qemu-system-i386: Could not install MSR_CORE_THREAD_COUNT handler: Success

2022-12-31 Thread Vitaly Chikunov

Alexander,

On Sat, Dec 31, 2022 at 10:28:21AM +0100, Alexander Graf wrote:
> On 30.12.22 19:16, Vitaly Chikunov wrote:
> > On Fri, Dec 30, 2022 at 06:44:14PM +0100, Alexander Graf wrote:
> > > 
> > > This is a kvm kernel bug and should be fixed with the latest stable 
> > > releases. Which kernel version are you running?
> > This is on latest v6.0 stable - 6.0.15.
> > 
> > Maybe there could be workaround for such situations? (Or maybe it's
> > possible to make this error non-fatal?) We use qemu+kvm for testing and
> > now we cannot test on x86.
> 
> I'm confused what's going wrong for you. I tried to reproduce the issue
> locally, but am unable to:
> 
> $ uname -a
> Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
> 2022 x86_64 x86_64 x86_64 GNU/Linux
> $ linux32 chroot .
> $ uname -a
> Linux server 6.0.15-default #1 SMP PREEMPT_DYNAMIC Sat Dec 31 07:52:52 CET
> 2022 i686 GNU/Linux
> $ cd qemu
> $ file ./build/qemu-system-i386
> ./build/qemu-system-i386: ELF 32-bit LSB shared object, Intel 80386, version
> 1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.2, for GNU/Linux
> 3.2.0, BuildID[sha1]=f75e20572be5c604c121de4497397665c168aa4c, with
> debug_info, not stripped
> $ ./build/qemu-system-i386 --version
> QEMU emulator version 7.2.0 (v7.2.0-dirty)
> Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
> $ ./build/qemu-system-i386 -nographic -enable-kvm
> SeaBIOS (version rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org)
> [...]
> 
> 
> Can you please double check whether your host kernel version is 6.0.15?
> Please paste the output of "uname -a".

Excuse me, I'm incorrectly reported kernel version I tried to boot instead
of host one. Host kernels are quite old, 5.15.59 and even 5.17.15 --
where failure is occurring.

I just tested on 5.15.85 and there is no failure.

  builder@i586:/.in$ uname -a
  Linux localhost.localdomain 5.15.85-std-def-alt1 #1 SMP Wed Dec 21 21:14:40 
UTC 2022 i686 GNU/Linux
  builder@i586:/.in$ qemu-system-i386 -nographic -enable-kvm
  SeaBIOS (version 1.16.1-alt1)

Perhaps, one of solutions it to reboot our build fleet to newer kernels.
[This maybe hard, though, since special builder node image should be
created and reboot shall be coordinated through all systems, in compare,
updating QEMU would be easier since chroot is created on every build].

Thanks for your helpful replies!

Thanks,

> 
> 
> Alex

Re: [PATCH v2 2/3] hw/mips/malta: Set PIIX4 IRQ routes in embedded bootloader

2022-12-31 Thread Bernhard Beschow




Am 21. November 2022 15:34:05 UTC schrieb Bernhard Beschow :
>
>
>Am 27. Oktober 2022 20:47:19 UTC schrieb "Philippe Mathieu-Daudé" 
>:
>>Linux kernel expects the northbridge & southbridge chipsets
>>configured by the BIOS firmware. We emulate that by writing
>>a tiny bootloader code in write_bootloader().
>>
>>Upon introduction in commit 5c2b87e34d ("PIIX4 support"),
>>the PIIX4 configuration space included values specific to
>>the Malta board.
>>
>>Set the Malta-specific IRQ routing values in the embedded
>>bootloader, so the next commit can remove the Malta specific
>>bits from the PIIX4 PCI-ISA bridge and make it generic
>>(matching the real hardware).
>>
>>Signed-off-by: Philippe Mathieu-Daudé 
>>---
>>FIXME: Missing the nanoMIPS counter-part!
>
>Who will be taking care of this? I have absolutely no clue how the 
>write_bootloader functions work, so I don't see how to fix it.

Ping

>Couldn't we just do it like in pegasos2_init() where the registers are 
>initialized by QEMU directly if there is no bootloader binary configured? I 
>could do that.
>
>Best regards,
>Bernhard
>>---
>> hw/mips/malta.c | 19 +++
>> 1 file changed, 19 insertions(+)
>>
>>diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>>index df0f448b67..4403028778 100644
>>--- a/hw/mips/malta.c
>>+++ b/hw/mips/malta.c
>>@@ -804,6 +804,8 @@ static void write_bootloader_nanomips(uint8_t *base, 
>>uint64_t run_addr,
>> stw_p(p++, 0x8422); stw_p(p++, 0x9088);
>> /* sw t0, 0x88(t1)  */
>> 
>>+/* TODO set PIIX IRQC[A:D] routing values! */
>>+
>> stw_p(p++, 0xe320 | NM_HI1(kernel_entry));
>> 
>> stw_p(p++, NM_HI2(kernel_entry));
>>@@ -841,6 +843,9 @@ static void write_bootloader_nanomips(uint8_t *base, 
>>uint64_t run_addr,
>> static void write_bootloader(uint8_t *base, uint64_t run_addr,
>>  uint64_t kernel_entry)
>> {
>>+const char pci_pins_cfg[PCI_NUM_PINS] = {
>>+10, 10, 11, 11 /* PIIX IRQRC[A:D] */
>>+};
>> uint32_t *p;
>> 
>> /* Small bootloader */
>>@@ -915,6 +920,20 @@ static void write_bootloader(uint8_t *base, uint64_t 
>>run_addr,
>> 
>> #undef cpu_to_gt32
>> 
>>+/*
>>+ * The PIIX ISA bridge is on PCI bus 0 dev 10 func 0.
>>+ * Load the PIIX IRQC[A:D] routing config address, then
>>+ * write routing configuration to the config data register.
>>+ */
>>+bl_gen_write_u32(, /* GT_PCI0_CFGADDR */
>>+ cpu_mips_phys_to_kseg1(NULL, 0x1be0 + 0xcf8),
>>+ tswap32((1 << 31) /* ConfigEn */
>>+ | PCI_BUILD_BDF(0, PIIX4_PCI_DEVFN) << 8
>>+ | PIIX_PIRQCA));
>>+bl_gen_write_u32(, /* GT_PCI0_CFGDATA */
>>+ cpu_mips_phys_to_kseg1(NULL, 0x1be0 + 0xcfc),
>>+ tswap32(ldl_be_p(pci_pins_cfg)));
>>+
>> bl_gen_jump_kernel(,
>>true, ENVP_VADDR - 64,
>>/*

Re: [PATCH qemu] x86: don't let decompressed kernel image clobber setup_data