Re: [PATCH 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[David Woodhouse]

On Thu, 2022-03-17 at 10:05 +0100, Igor Mammedov wrote:
> re-sending reply as something went wrong with headers (I suspect Daniel's 
> name formatting)
> and email got bounced back.
> 
> On Wed, 16 Mar 2022 14:31:33 +0000
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > On Wed, 2022-03-16 at 12:28 +0100, Igor Mammedov wrote:  
> > > Generally Daniel is right, as long as it's something that what real 
> > > hardware
> > > supports. (usually it's job if upper layers which know what guest OS is 
> > > used,
> > > and can tweak config based on that knowledge).
> > > 
> > > But it's virt only extension and none (tested with
> > >  Windows (hangs on boot),
> > >  Linux (brings up only first 255 cpus)
> > > ) of mainline OSes ended up up working as expected (i.e. user asked for 
> > > this
> > > many CPUs but can't really use them as expected).    
> > 
> > As I said, that kind of failure mode will happen even with the split
> > irq chip and EXT_DEST_ID, with Windows and older (pre-5.10) Linux
> > kernels.
> > 
> > For older guests it would also happen on real hardware, and in VMs
> > where you expose an IOMMU with interrupt remapping. Some guests don't
> > support interrupt remapping, or don't support X2APIC at all.
> >   
> > > Which would just lead to users reporting (obscure) bugs.    
> > 
> > It's not virt only. This could happen with real hardware.  
> 
> I was talking about EXT_DEST_ID kvm extension.

Then I'm confused, because that isn't the conversation we were having
before. And in that case what you say above about Linux only bringing
up the first 255 CPUs directly contradicts what you say below, my own
experience, and the whole *point* of the EXT_DEST_ID extension :)

Let's start again. You observed that Linux guests fail to bring up >254
vcPUs if qemu doesn't enable the EXT_DEST_ID support, and your fix was
to require EXT_DEST_ID (as a side-effect of requiring split irqchip).

This reminded me of the fixes I'd been posting since 2020 which still
haven't been merged, so I dusted those off and resent them.

I didn't incorporate your change, and objected to your patch because I
think it's pointless babysitting. Yes, in the general case if you want
your guest to use more than 254 vCPUs you need to take a moment to
think about precisely what your guest operating system requires in
order to support that.

At the very least it needs X2APIC support, and then you need *one* of:

 • EXT_DEST_ID,
 • Interrupt remapping, or
 • just using those vCPUs without external interrupts.

Both of the first two require the split irqchip, so your patch just
doesn't let users rely on that last option. I conceded (cited below)
because I don't know of any existing guest OS which does use that last
option. I'd attempted to make Linux do so, but eventually abandoned it:
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/irqaffinity

But now you seem to be making a different argument?

> > Anyway, as far as I'm concerned it doesn't matter very much whether we
> > insist on the split irq chip or not. Feel free to repost your patch
> > rebased on top of my fixes, which are also in my tree at
> > https://git.infradead.org/users/dwmw2/qemu.git
> > 
> > 
> > The check you're modifying has moved to x86_cpus_init().  
> 
> if we are to keep iommu dependency then moving to x86_cpus_init()
> isn't an option, it should be done at pc_machine_done() time.

Thus far, I didn't think anyone had been talking about a dependency on
IOMMU. That doesn't make any sense at all. EXT_DEST_ID is perfectly
sufficient for Linux kernels from 5.10 onwards and they don't need the
IOMMU.

So no. Post your patch to s/kvm_irqchip_in_kernel/kvm_irqchip_is_split/
in x86_cpus_init() by all means; I don't care much about that and I've
even incorporated that change into my tree at
https://git.infradead.org/users/dwmw2/qemu.git so that if I do have to
repost these fixes yet again, it'll be included.

But let's not re-add the IOMMU dependency. That would be wrong.

smime.p7s
Description: S/MIME cryptographic signature