Linux Virtualization - Oct 2017 - [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky
wrote:
> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>> Maybe we can add a new field to the alternatives entry struct
which
> >>> specifies the offset to the CALL instruction, so
apply_alternatives()
> >>> can find it.
> >> We'd also have to assume that the restore part of an
alternative entry
> >> is the same size as the save part. Which is true now.
> > Why?
> >
> 
> Don't you need to know the size of the instruction without save and
> restore part?
> 
> + if (a->replacementlen == 6 && *insnbuf == 0xff &&
*(insnbuf+1) == 0x15)
> 
> Otherwise you'd need another field for the actual instruction length.
If we know where the CALL instruction starts, and can verify that it
starts with "ff 15", then we know the instruction length: 6 bytes.
Right?

-- 
Josh

On 10/17/2017 04:50 PM, Josh Poimboeuf wrote:
> On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
>> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
>>> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
>>>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
>>>>> Maybe we can add a new field to the alternatives entry
struct which
>>>>> specifies the offset to the CALL instruction, so
apply_alternatives()
>>>>> can find it.
>>>> We'd also have to assume that the restore part of an
alternative entry
>>>> is the same size as the save part. Which is true now.
>>> Why?
>>>
>> Don't you need to know the size of the instruction without save and
>> restore part?
>>
>> + if (a->replacementlen == 6 && *insnbuf == 0xff &&
*(insnbuf+1) == 0x15)
>>
>> Otherwise you'd need another field for the actual instruction
length.
> If we know where the CALL instruction starts, and can verify that it
> starts with "ff 15", then we know the instruction length: 6
bytes.
> Right?
>
Oh, OK. Then you shouldn't need a->replacementlen test(s?) in
apply_alternatives()?

-boris

On Tue, Oct 17, 2017 at 04:59:41PM -0400, Boris Ostrovsky
wrote:
> On 10/17/2017 04:50 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> >>> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky
wrote:
> >>>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>>>> Maybe we can add a new field to the alternatives entry
struct which
> >>>>> specifies the offset to the CALL instruction, so
apply_alternatives()
> >>>>> can find it.
> >>>> We'd also have to assume that the restore part of an
alternative entry
> >>>> is the same size as the save part. Which is true now.
> >>> Why?
> >>>
> >> Don't you need to know the size of the instruction without
save and
> >> restore part?
> >>
> >> + if (a->replacementlen == 6 && *insnbuf == 0xff
&& *(insnbuf+1) == 0x15)
> >>
> >> Otherwise you'd need another field for the actual instruction
length.
> > If we know where the CALL instruction starts, and can verify that it
> > starts with "ff 15", then we know the instruction length: 6
bytes.
> > Right?
> >
> 
> Oh, OK. Then you shouldn't need a->replacementlen test(s?) in
> apply_alternatives()?
Right.  Though in the above code it was needed for a different reason,
to make sure it wasn't reading past the end of the buffer.

-- 
Josh