Re: [Qemu-devel] [PATCH v7 7/9] i386: Add support for CPUID_8000_001E for AMD

[Eduardo Habkost]

On Fri, May 11, 2018 at 02:44:11PM +0000, Moger, Babu wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:address@hidden
> > Sent: Friday, May 11, 2018 9:12 AM
> > To: Moger, Babu <address@hidden>
> > Cc: address@hidden; address@hidden; address@hidden;
> > address@hidden; address@hidden; address@hidden;
> > address@hidden; address@hidden; address@hidden
> > Subject: Re: [Qemu-devel] [PATCH v7 7/9] i386: Add support for
> > CPUID_8000_001E for AMD
> > 
> > On Tue, May 08, 2018 at 03:02:07PM +0000, Moger, Babu wrote:
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost [mailto:address@hidden
> > > > Sent: Tuesday, May 8, 2018 9:17 AM
> > > > To: Moger, Babu <address@hidden>
> > > > Cc: address@hidden; address@hidden; address@hidden;
> > > > address@hidden; address@hidden; address@hidden;
> > > > address@hidden; address@hidden; address@hidden
> > > > Subject: Re: [Qemu-devel] [PATCH v7 7/9] i386: Add support for
> > > > CPUID_8000_001E for AMD
> > > >
> > > > On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote:
> > > > [...]
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Encode cache info for CPUID[0x80000006].ECX and
> > > > > > CPUID[0x80000006].EDX
> > > > > > >   * @l3 can be NULL.
> > > > > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env,
> > > > > > uint32_t index, uint32_t count,
> > > > > > >              break;
> > > > > > >          }
> > > > > > >          break;
> > > > > > > +    case 0x8000001E:
> > > > > > > +        assert(cpu->core_id <= 255);
> > > > > >
> > > > > > Where's the code that ensures this assert() line can't be
> > > > > > triggered by any command-line configuration?
> > > > >
> > > > > I did not understand this. Can you please elaborate. Thanks
> > > >
> > > > The user must not be able to trigger an assert(), so we need to
> > > > ensure that core_id will never be larger than 255.  Is there
> > > > existing code that ensures that?
> > >
> > > I see that max_cpus is set to 255 and also there are checks to make sure
> > core_id does not go above 255.
> > > I verified it while testing.  So, probably we don't need assert here.  
> > > Radim
> > asked me to add this assert.
> > > I can remove it if no abjections.
> > 
> > Sorry for not replying to this before: no objection to the
> > assert(), especially considering it will trigger very early on
> > initialization if we break it one day.
> 
> Ok. No problem.  I will add it back and send a v9 version. 
> Please let me know if you have any other feedback for v8 version(sent 
> yesterday).

Thanks, I just saw v8 on my mailbox.  No need to send v9 just
because of the assert(), I can re-add it while committing if
necessary.

-- 
Eduardo