Re: [Xen-devel] [PATCH 00/11] pl011 emulation support in Xen

[Bhupinder Thakur]

Hi,


On 5 March 2017 at 12:46, Julien Grall  wrote:
> Hi Bhupinder,
>
> On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
>>
>> There are still some items which are pending:
>>
>> 1. Adding dynamic enable/disable of pl011 emulation for a guest
>> 2. Add a new console type "pl011" in xenconsoled to allow the user to
>> connect to
>> either PV/serial/pl011 console.
>> 3. Add checks to ensure that the new hvm params read/written by the guest
>
>
> A couple of recommendations regarding the series:
> - All maintainers of the code you touch in a patch should be CCed.
> You can use scripts/get_maintainters.pl for that.

I will run the script before sending the patches.

> - Providing a git branch with your code will allow people to test
> your code without handling potential rebasing.
>
I plan to check-in the code in a linaro git repository and will share
the branch information in the next patch.

> Cheers,
>
> --
> Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 00/11] pl011 emulation support in Xen

[Bhupinder Thakur]

Hi,

Thanks for your comments.

On 3 March 2017 at 21:23, Konrad Rzeszutek Wilk  wrote:
>> The following changes were done:
>
> .. snip..
>
> Thank you for this great writeup. I took a stab at it and stopped at patch
> #2 b/c Julien said he would look in it deeper. But based on a brief
> look I would say:
>  - Please do remove most of the comments. They really do not add
>much context besides describing the code - and we all can
>read the code. The idea behind the comments is to describe some
>semantics of it, or something that is not obvious at first or
>such. Not the code.
I will remove the comments where not required.

>
>  - Comments. One line comments are:
>   /* Comment. */
>And please do use proper case and a period.
>
I will correct the comments.

>  - Be careful about compiler optimizations and jump tables.
>Specifically see https://xenbits.xen.org/xsa/advisory-155.html
>The way to make sure you don't introduce an security problem
>is to 1) use local variables 2) read once from the ring and
>make sure you use a compiler barrier.
>
will check the guidelines.

>  - There is also some unrelated changes. Like extra newlines. One
>way to avoid this is to send all your patches _just_ to yourself
>and review them - but review them in reverse order and from the
>bottom of the emails to the top. That way you can catch some of this.
>
>  - Think in terms of how one would break this. For example the guest
>could change the HVM parameters (or maybe not?) - or find the
>console ring and muck with the ring indexes. You need to shield
>the code from such changes.
>
I plan to add the checks that the HVM params can only be accessed from
a privileged guest.

> Thanks!

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 00/11] pl011 emulation support in Xen

[Julien Grall]

Hi Bhupinder,

On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:

There are still some items which are pending:

1. Adding dynamic enable/disable of pl011 emulation for a guest
2. Add a new console type "pl011" in xenconsoled to allow the user to connect to
either PV/serial/pl011 console.
3. Add checks to ensure that the new hvm params read/written by the guest


A couple of recommendations regarding the series:
	- All maintainers of the code you touch in a patch should be CCed. You 
can use scripts/get_maintainters.pl for that.
	- Providing a git branch with your code will allow people to test your 
code without handling potential rebasing.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 00/11] pl011 emulation support in Xen

[Konrad Rzeszutek Wilk]

On Fri, Mar 03, 2017 at 03:23:31PM -0500, Konrad Rzeszutek Wilk wrote:
> > The following changes were done:
> 
> .. snip..
> 
> Thank you for this great writeup. I took a stab at it and stopped at patch
> #2 b/c Julien said he would look in it deeper. But based on a brief
> look I would say:

Run this on each patch:

$more bin/add_c 
#!/bin/bash

git diff HEAD^.. > /tmp/a

echo "---"
cat /tmp/a | scripts/get_maintainer.pl --no-l  | while read file; do echo "Cc: 
$file";done
rm -f /tmp/a

So that you have the right maintainers on the CC line 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 00/11] pl011 emulation support in Xen

[Konrad Rzeszutek Wilk]

> The following changes were done:

.. snip..

Thank you for this great writeup. I took a stab at it and stopped at patch
#2 b/c Julien said he would look in it deeper. But based on a brief
look I would say:
 - Please do remove most of the comments. They really do not add
   much context besides describing the code - and we all can
   read the code. The idea behind the comments is to describe some
   semantics of it, or something that is not obvious at first or
   such. Not the code.

 - Comments. One line comments are:
  /* Comment. */
   And please do use proper case and a period.

 - Be careful about compiler optimizations and jump tables.
   Specifically see https://xenbits.xen.org/xsa/advisory-155.html
   The way to make sure you don't introduce an security problem
   is to 1) use local variables 2) read once from the ring and
   make sure you use a compiler barrier.

 - There is also some unrelated changes. Like extra newlines. One
   way to avoid this is to send all your patches _just_ to yourself
   and review them - but review them in reverse order and from the
   bottom of the emails to the top. That way you can catch some of this.

 - Think in terms of how one would break this. For example the guest
   could change the HVM parameters (or maybe not?) - or find the
   console ring and muck with the ring indexes. You need to shield
   the code from such changes.

Thanks!

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel