Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

On Fri, Feb 27, 2015 at 08:23:12AM +0900, Peter Maydell wrote:
> On 27 February 2015 at 08:11, Eduardo Habkost <address@hidden> wrote:
> > On Fri, Feb 27, 2015 at 08:00:25AM +0900, Peter Maydell wrote:
> >> On 27 February 2015 at 07:51, Eduardo Habkost <address@hidden> wrote:
> >> > I have never seen it in practice, but in x86 it will fail if we try to
> >> > create more than 254 VCPUs.
> >>
> >> That's bad -- it's not hard to imagine a program with 256
> >> threads. Why does x86 have this limitation and can we fix it?
> >> That seems like the better course than this patch.
> >
> > I don't think *-user or any x86-specific code has any hard requirement
> > of having all VCPUs with different APIC IDs. But right now we set APIC
> > ID = cpu_index for every VCPU, so the current code has this limitation.
> 
> But the -user code doesn't have an APIC at all, so it shouldn't
> have to live with this limitation.

Yes. I don't know yet if we can safely set apic_id=0 on all usermode
VCPUs (maybe some existing code will break if we do that), but we should
be able to do it eventually.

> 
> > I would be glad to send a patch adding an assert() instead of an error
> > message, but I don't think I will be able to audit every single
> > cpu_init() function soon (to ensure they really never fail).
> >
> > So, while we don't audit all cpu_init() functions, do you prefer to live
> > with an error message, or an assert() that may or may not be triggered
> > by existing code?
> 
> I would prefer:
>  (1) that we fix the error handling properly so you return an error
>      indication from cpu_copy() and we propagate it back to the guest
>  (2) that we fix x86-64 so it can create more than 254 CPUs for usermode
> 
> I would rather not paper over this by adding a "let qemu assert or
> fatally exit on a recoverable error condition" code path we'd just
> have to revert when we fixed (1) properly.
> 
> (2) is not really important to deal with right now, since we don't
> actually support multiple threads yet. But it would be nice eventually.

So, I have taken a look at the cpu_init() implementations, and all of
them seem to never fail unless an invalid CPU model string is used,
except for i386.

So it looks like there's no compelling reason to write extra code for
(1) right now if we fix (2). On the other hand, we will always have the
possibility of somebody introducing additional cpu_init() error
conditions in some architecture in the future (that's why I suggested
adding an assert()).

Personally, I don't mind either way. I will just leave this for the
developers interested in linux-user.

-- 
Eduardo