Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 10/15/18 10:23 AM, Paolo Bonzini wrote: > > Even for a value from a 32-bit register? That would be _BIT, which > doesn't exist. > Just use _BITUL(). gcc is smart enough to know that that the resulting value is representable in 32 bits. Or if you really care, submit a patch to create _BITU(), but I don't personally see much of a point. -hpa
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 10/15/18 10:23 AM, Paolo Bonzini wrote: > > Even for a value from a 32-bit register? That would be _BIT, which > doesn't exist. > Just use _BITUL(). gcc is smart enough to know that that the resulting value is representable in 32 bits. Or if you really care, submit a patch to create _BITU(), but I don't personally see much of a point. -hpa
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 15/10/2018 19:16, H. Peter Anvin wrote: > On 10/7/18 6:04 PM, peng.h...@zte.com.cn wrote: > \> >> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF) >> -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31) >> +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31) > It is reasonable to change to unsigned, while not necessary to unsigned > long? AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write. here I think it doesn't matter if you use unsigned or unsigned long. Do you have any suggestions? >> >>> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate >>> the value of new_entry with type of u32. So the definition here is not >>> harmful. >> >>> Also, I did a quick grep and found similar definition (1 << 31) is popular >>> in the whole kernel tree. >> >>> The reason to make this change is not that strong to me. Would you >>> minding sharing more reason behind this change? >> oh, I'm just thinking logically, not more reason. > > The right way to do this would be to use the _BITUL() (or _BITULL()) macro. Even for a value from a 32-bit register? That would be _BIT, which doesn't exist. Paolo
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 15/10/2018 19:16, H. Peter Anvin wrote: > On 10/7/18 6:04 PM, peng.h...@zte.com.cn wrote: > \> >> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF) >> -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31) >> +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31) > It is reasonable to change to unsigned, while not necessary to unsigned > long? AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write. here I think it doesn't matter if you use unsigned or unsigned long. Do you have any suggestions? >> >>> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate >>> the value of new_entry with type of u32. So the definition here is not >>> harmful. >> >>> Also, I did a quick grep and found similar definition (1 << 31) is popular >>> in the whole kernel tree. >> >>> The reason to make this change is not that strong to me. Would you >>> minding sharing more reason behind this change? >> oh, I'm just thinking logically, not more reason. > > The right way to do this would be to use the _BITUL() (or _BITULL()) macro. Even for a value from a 32-bit register? That would be _BIT, which doesn't exist. Paolo
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 10/7/18 6:04 PM, peng.h...@zte.com.cn wrote: \> > #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF) > -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31) > +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31) >>> It is reasonable to change to unsigned, while not necessary to unsigned long? >>> AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write. >>> here I think it doesn't matter if you use unsigned or unsigned long. Do you >>> have any suggestions? > >> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate >> the value of new_entry with type of u32. So the definition here is not >> harmful. > >> Also, I did a quick grep and found similar definition (1 << 31) is popular >> in the whole kernel tree. > >> The reason to make this change is not that strong to me. Would you >> minding sharing more reason behind this change? > oh, I'm just thinking logically, not more reason. The right way to do this would be to use the _BITUL() (or _BITULL()) macro. -hpa
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 10/7/18 6:04 PM, peng.h...@zte.com.cn wrote: \> > #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF) > -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31) > +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31) >>> It is reasonable to change to unsigned, while not necessary to unsigned long? >>> AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write. >>> here I think it doesn't matter if you use unsigned or unsigned long. Do you >>> have any suggestions? > >> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate >> the value of new_entry with type of u32. So the definition here is not >> harmful. > >> Also, I did a quick grep and found similar definition (1 << 31) is popular >> in the whole kernel tree. > >> The reason to make this change is not that strong to me. Would you >> minding sharing more reason behind this change? > oh, I'm just thinking logically, not more reason. The right way to do this would be to use the _BITUL() (or _BITULL()) macro. -hpa
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 08/10/2018 04:25, Wei Yang wrote:
> On Mon, Oct 08, 2018 at 09:04:34AM +0800, peng.h...@zte.com.cn wrote:
>>> On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
> On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>>
>> From: Peng Hao
>>
>> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>>
>> Signed-off-by: Peng Hao
>> ---
>> arch/x86/kvm/svm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index d96092b..bf1ded4 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
>> };
>>
>> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>> -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>> +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
> It is reasonable to change to unsigned, while not necessary to unsigned
> long?
AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
here I think it doesn't matter if you use unsigned or unsigned long. Do
you have any suggestions?
>>
>>> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
>>> the value of new_entry with type of u32. So the definition here is not
>>> harmful.
>>
>>> Also, I did a quick grep and found similar definition (1 << 31) is popular
>>> in the whole kernel tree.
>>
>>> The reason to make this change is not that strong to me. Would you
>>> minding sharing more reason behind this change?
>> oh, I'm just thinking logically, not more reason.
>
> This definition may introduce problem when this value is used to
> calculate a 64bit data.
>
> Since current entry is 32bit, we may leave it as it is for now.
I agree.
Paolo
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On 08/10/2018 04:25, Wei Yang wrote:
> On Mon, Oct 08, 2018 at 09:04:34AM +0800, peng.h...@zte.com.cn wrote:
>>> On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
> On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>>
>> From: Peng Hao
>>
>> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>>
>> Signed-off-by: Peng Hao
>> ---
>> arch/x86/kvm/svm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index d96092b..bf1ded4 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
>> };
>>
>> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>> -#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>> +#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
> It is reasonable to change to unsigned, while not necessary to unsigned
> long?
AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
here I think it doesn't matter if you use unsigned or unsigned long. Do
you have any suggestions?
>>
>>> In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
>>> the value of new_entry with type of u32. So the definition here is not
>>> harmful.
>>
>>> Also, I did a quick grep and found similar definition (1 << 31) is popular
>>> in the whole kernel tree.
>>
>>> The reason to make this change is not that strong to me. Would you
>>> minding sharing more reason behind this change?
>> oh, I'm just thinking logically, not more reason.
>
> This definition may introduce problem when this value is used to
> calculate a 64bit data.
>
> Since current entry is 32bit, we may leave it as it is for now.
I agree.
Paolo
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Mon, Oct 08, 2018 at 09:04:34AM +0800, peng.h...@zte.com.cn wrote:
>>On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>
>From: Peng Hao
>
> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>
>Signed-off-by: Peng Hao
>---
> arch/x86/kvm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>index d96092b..bf1ded4 100644
>--- a/arch/x86/kvm/svm.c
>+++ b/arch/x86/kvm/svm.c
>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
> };
>
> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
>>>
It is reasonable to change to unsigned, while not necessary to unsigned
long?
>>>AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
>>>here I think it doesn't matter if you use unsigned or unsigned long. Do you
>>>have any suggestions?
>
>>In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
>>the value of new_entry with type of u32. So the definition here is not
>>harmful.
>
>>Also, I did a quick grep and found similar definition (1 << 31) is popular
>>in the whole kernel tree.
>
>>The reason to make this change is not that strong to me. Would you
>>minding sharing more reason behind this change?
>oh, I'm just thinking logically, not more reason.
This definition may introduce problem when this value is used to
calculate a 64bit data.
Since current entry is 32bit, we may leave it as it is for now.
--
Wei Yang
Help you, Help me
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Mon, Oct 08, 2018 at 09:04:34AM +0800, peng.h...@zte.com.cn wrote:
>>On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>
>From: Peng Hao
>
> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>
>Signed-off-by: Peng Hao
>---
> arch/x86/kvm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>index d96092b..bf1ded4 100644
>--- a/arch/x86/kvm/svm.c
>+++ b/arch/x86/kvm/svm.c
>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
> };
>
> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
>>>
It is reasonable to change to unsigned, while not necessary to unsigned
long?
>>>AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
>>>here I think it doesn't matter if you use unsigned or unsigned long. Do you
>>>have any suggestions?
>
>>In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
>>the value of new_entry with type of u32. So the definition here is not
>>harmful.
>
>>Also, I did a quick grep and found similar definition (1 << 31) is popular
>>in the whole kernel tree.
>
>>The reason to make this change is not that strong to me. Would you
>>minding sharing more reason behind this change?
>oh, I'm just thinking logically, not more reason.
This definition may introduce problem when this value is used to
calculate a 64bit data.
Since current entry is 32bit, we may leave it as it is for now.
--
Wei Yang
Help you, Help me
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
>>On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>>>
>>>From: Peng Hao
>>>
>>> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>>>
>>>Signed-off-by: Peng Hao
>>>---
>>> arch/x86/kvm/svm.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>index d96092b..bf1ded4 100644
>>>--- a/arch/x86/kvm/svm.c
>>>+++ b/arch/x86/kvm/svm.c
>>>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
>>> };
>>>
>>> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>>>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>>>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
>
>>It is reasonable to change to unsigned, while not necessary to unsigned
>>long?
>AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
>here I think it doesn't matter if you use unsigned or unsigned long. Do you
>have any suggestions?
In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
the value of new_entry with type of u32. So the definition here is not
harmful.
Also, I did a quick grep and found similar definition (1 << 31) is popular
in the whole kernel tree.
The reason to make this change is not that strong to me. Would you
minding sharing more reason behind this change?
>>--
>>Wei Yang
>>Help you, Help me
--
Wei Yang
Help you, Help me
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Sat, Oct 06, 2018 at 11:31:04AM +0800, peng.h...@zte.com.cn wrote:
>>On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>>>
>>>From: Peng Hao
>>>
>>> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>>>
>>>Signed-off-by: Peng Hao
>>>---
>>> arch/x86/kvm/svm.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>index d96092b..bf1ded4 100644
>>>--- a/arch/x86/kvm/svm.c
>>>+++ b/arch/x86/kvm/svm.c
>>>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
>>> };
>>>
>>> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK(0xFF)
>>>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1 << 31)
>>>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK(1UL << 31)
>
>>It is reasonable to change to unsigned, while not necessary to unsigned
>>long?
>AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used in function avic_ldr_write.
>here I think it doesn't matter if you use unsigned or unsigned long. Do you
>have any suggestions?
In current case, AVIC_LOGICAL_ID_ENTRY_VALID_MASK is used to calculate
the value of new_entry with type of u32. So the definition here is not
harmful.
Also, I did a quick grep and found similar definition (1 << 31) is popular
in the whole kernel tree.
The reason to make this change is not that strong to me. Would you
minding sharing more reason behind this change?
>>--
>>Wei Yang
>>Help you, Help me
--
Wei Yang
Help you, Help me
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>
>From: Peng Hao
>
> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>
>Signed-off-by: Peng Hao
>---
> arch/x86/kvm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>index d96092b..bf1ded4 100644
>--- a/arch/x86/kvm/svm.c
>+++ b/arch/x86/kvm/svm.c
>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
> };
>
> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK (0xFF)
>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1 << 31)
>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1UL << 31)
It is reasonable to change to unsigned, while not necessary to unsigned
long?
>
> #define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK (0xFFULL)
> #define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK (0xFFULL << 12)
>--
>1.8.3.1
>
--
Wei Yang
Help you, Help me
Re: [PATCH] kvm/x86 : avoid shifting signed 32-bit value by 31 bits
On Thu, Oct 04, 2018 at 01:47:18PM -0400, Peng Hao wrote:
>
>From: Peng Hao
>
> modify AVIC_LOGICAL_ID_ENTRY_VALID_MASK to unsigned
>
>Signed-off-by: Peng Hao
>---
> arch/x86/kvm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>index d96092b..bf1ded4 100644
>--- a/arch/x86/kvm/svm.c
>+++ b/arch/x86/kvm/svm.c
>@@ -262,7 +262,7 @@ struct amd_svm_iommu_ir {
> };
>
> #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK (0xFF)
>-#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1 << 31)
>+#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1UL << 31)
It is reasonable to change to unsigned, while not necessary to unsigned
long?
>
> #define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK (0xFFULL)
> #define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK (0xFFULL << 12)
>--
>1.8.3.1
>
--
Wei Yang
Help you, Help me
